docs(compass): allow linking held chains; answer review findings with new commits - #315
Conversation
Linking lands nothing: the landing rule already refuses to merge past a held PR, since it requires every PR below the target to be unlabelled. Linking itself is harmless, and the ban was blocking stacks the owner wants. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Of the 14 force-pushes on open PRs since the no-force-push rule was added, at least 10 broke it. Almost all of them answered review findings by amending commits. Branches land squashed, so an amend gains nothing. It costs the review its base commit, so no delta review is possible, and it breaks children: #66 still carries #62's pre-amend commit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| re-run whenever a PR joins, never with `need human` below. `merge` retargets | ||
| only linked members. Drift check: every open PR based on another open PR's | ||
| branch sits in one stack (`gh api "repos/<o>/<r>/stacks?pull_request=<n>"`). | ||
| re-run whenever a PR joins, held members included (linking lands nothing). |
There was a problem hiding this comment.
Blocking — this line contradicts the label rule at L103-104, which the PR leaves unchanged.
L103-104 still says the label "stops all agent action on that issue or PR (no commit, review, amend or merge, even after a passed review)". Linking a held member is agent action on that PR. The parenthetical does not list linking, but it gives examples and does not narrow "all". The PR body reads the rule as its list ("the label still forbids commits, reviews, amends and merges"). The file does not say that, and L155 says "this file wins".
Linking also does more than the parenthetical's reading allows:
gh stack linkwith branch arguments pushes them first.gh stack link --help: "Branch arguments are automatically pushed to the remote before creating or looking up PRs."<bottom> ... <top>accepts branch names, so linking a held member from a stale or ahead local branch is a push to a held PR.- Once a held PR is linked,
gh stack mergeon a PR below it retargets the held PR's base with no further command (L177, and measured on probe: ghstack A #130/probe: ghstack B #131). - The help documents base chaining only for PRs it creates. I did not test whether it re-bases an existing PR, and I did not run
linkhere.
Fix, one clause at L103: add a named exception, e.g. "(no commit, review, amend or merge, even after a passed review; linking it into a stack by PR number, and the base retarget that follows a landing below it, are allowed)". Separately, spell the command here as PR numbers, not branches.
There was a problem hiding this comment.
Fixed in a8377a4: L103 now names the exception ("with one exception: gh stack link by PR number, which lands and pushes nothing"), and the link command takes PR numbers, not branch names.
There was a problem hiding this comment.
Resolved at a8377a446, as far as round 1's stated fix goes. L103-105 names the exception, and L176 takes PR numbers. I checked "pushes nothing" against the gh stack v0.1.1 source (pushBranchArgs pushes only arguments that exist as local branches, and numeric arguments resolve as PRs first). The claim holds.
Still open, not blocking: link also rewrites each listed PR's base (fixBaseBranches), and the exception does not name the retarget that a landing below causes. Details are in the new thread on L105.
| only linked members. Drift check: every open PR based on another open PR's | ||
| branch sits in one stack (`gh api "repos/<o>/<r>/stacks?pull_request=<n>"`). | ||
| re-run whenever a PR joins, held members included (linking lands nothing). | ||
| `merge` retargets only linked members. Drift check: every open PR based on |
There was a problem hiding this comment.
Non-blocking — once a held chain is linked, "linking lands nothing" rests on the landing rule alone.
gh stack merge checks "only basic pull request state ... (open and not a draft)" (its --help). It does not check labels. Before this PR a held member was never in a stack, so gh stack merge <n> above it, or gh stack merge <stack-number> --yes, could not include it. After this PR both commands land a held member atomically together with the rest of the stack. The landing rule (L147-152) still forbids that, and today every held member is a draft, which merge refuses. So nothing is wrong now. What changed is that the only guard left is agents obeying the rule, not the tool refusing. Consider one clause in the landing bullet: "gh stack merge takes the highest landable PR number, never a stack number."
There was a problem hiding this comment.
Fixed in a8377a4: the gh stack merge bullet now says it checks no labels, so the landing rule has to be applied first.
There was a problem hiding this comment.
Resolved at a8377a446. L183-184 now says merge checks no labels, so the landing rule is applied first. This matches gh stack merge --help ("Only basic pull request state is checked before merging (open and not a draft)").
| re-run whenever a PR joins, held members included (linking lands nothing). | ||
| `merge` retargets only linked members. Drift check: every open PR based on | ||
| another open PR's branch sits in one stack | ||
| (`gh api "repos/<o>/<r>/stacks?pull_request=<n>"`). |
There was a problem hiding this comment.
Non-blocking — the drift check now covers held chains, but the two held trees it will flag are forks, and "whole or not at all" has no rule for a fork.
The drift check was always unconditional. The old "never with need human below" is what kept held trees out of it in practice: #305's brief lists them as exclusions "since the label forbids linking". With that ban gone, the check now demands a stack for every held PR based on another open PR's branch. The two trees it flags are both forked:
- CA:
compass/ca-2-grant-ruleis the base of compass: both clock transports from one implementation (CA-3, #46) #63, compass: what a run of the clock says about itself (CA-7, #50) #67 and compass: ATOM's deployments driven against the clock, synthetically (CA-4, #47) #75. - IR:
compass/ir-1-data-modelis the base of compass(ir): the applicability predicate #61 and compass(ir): find the nested repetition in a flat block sequence #62.
A fork cannot sit "in one stack" linearly, so the next drift sweep will report these 9 PRs (the count is from #305's follow-up) and the file gives no remedy. The gap for forks predates this PR. This PR is what makes it reachable. Either say what the drift check does with a fork (exempt it and report it, or link one arm), or file it as an issue. It does not block this text.
There was a problem hiding this comment.
Fixed in a8377a4: a fork links at most one arm, and the drift check excepts the other arms.
There was a problem hiding this comment.
Addressed at a8377a446, but the new definition introduces a blocking contradiction. "A fork (two open PRs on one base)" makes feature/atomcompass_new a fork. Stacks 138 and 134 are both linked on it, and the text now allows only one. See the new thread on L178. The fix is to define a fork as "two open PRs based on one open PR's branch".
| - **Never force-push a branch under review. A restack after its parent has | ||
| landed is permitted.** | ||
| - **Never force-push a branch under review.** Answer review findings with new | ||
| commits, never an amend: the branch lands squashed anyway, and an amend removes |
There was a problem hiding this comment.
Blocking — L97, which this PR does not touch, still says "After each push a reviewer agent reviews, the developer amends, and that repeats until the verdict is APPROVE."
That line is the review loop's own description, and it now tells the developer to do exactly what this sentence forbids. An agent reading either line alone is told the opposite of the other. This is the same wording the 10 violations cited in the PR body follow. Fix, one word at L97: "the developer amends" → "the developer pushes fixes as new commits" (or "revises").
The rest of the sentence is consistent with the file. The delta review <approved sha>..<head> (L165-169) needs the approved commit in the head's ancestry, and new commits keep it there. The merge-tree check's --merge-base <parent's reviewed head> (L157-158) needs the same. The unlinked-chain rebase --onto (L183-184) is still covered by "a restack after the parent has landed". "Lands squashed anyway" matches L145.
There was a problem hiding this comment.
Fixed in a8377a4: L97 now says "the developer pushes fixes".
There was a problem hiding this comment.
Resolved at a8377a446. L97 now says "the developer pushes fixes", which agrees with L188-191. The only other "amend" is L104's list of what the label stops, and it contradicts nothing.
|
Agent-authored review. Verdict: CHANGES REQUESTED — the rules file ends up contradicting itself in two places the PR does not touch. The label rule (L103-104) still forbids "all agent action" on a held PR, and the review-loop line (L97) still says "the developer amends". Covers head Nothing was pushed, amended, merged, labelled, linked or retargeted. Principles and Scope in Blocking (inline)
Both fixes are one line each. With them in, I expect to approve on a delta review. Not blocking (inline)
What the review question asked, answered
Evidence checks on the PR body (GraphQL
|
… fixes Answers the review of b23cb85: - The need-human rule named a `gh stack link` exception nowhere, so it still forbade the linking that the stacking rule allows. It now names the exception: link by PR number, which lands and pushes nothing. - The review-loop line said "the developer amends", contradicting the new-commits rule. It now says "pushes fixes". - `gh stack merge` checks no labels, so the landing rule has to be applied first. - A fork links at most one arm, and the drift check excepts the other arms. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| branch sits in one stack (`gh api "repos/<o>/<r>/stacks?pull_request=<n>"`). | ||
| refuse): `gh stack link --base feature/atomcompass_new <bottom-pr#> ... <top-pr#>`, | ||
| re-run whenever a PR joins, held members included (linking lands nothing). | ||
| `merge` retargets only linked members. A fork (two open PRs on one base) links |
There was a problem hiding this comment.
Blocking — the fork definition makes the integration branch a fork, so the file now allows only one stack in the repo.
"A fork (two open PRs on one base)": feature/atomcompass_new is the base of #81, #85, #266, #292, #312, #314 and #315, so by this definition it is a fork. "Links at most one arm" then allows one linked chain off the integration branch. Every other chain is an arm that "stays unlinked" and is exempt from the drift check.
That contradicts the file and the live state:
- Two stacks are already linked on that base: 138 (compass(backends): a step price that reads the batch shape, and the schedule that moves with it (M1-2) #81→compass(backends): the stand-in model, its widths declared once (M1-3, #24) #120→compass(backends): charge the collective on the layers a worker runs #140→compass(backends): describe the collective term's stand-in note as it is emitted #148→compass(runner): the projection that builds a BatchView, and the rung it supplies #252) and 134 (compass(design): guard the open-items register's own counts #85→compass(design): a third document may not state the register's extent #95→compass(design): pin the register guard's one-id escape hatch #109→compass(tests): let a digit glued to a separator stay part of its identifier #118→compass(design): the one-id allowance bounds a span, not a document #119). Both are from
gh api "repos/jgong5/ATOM/stacks?pull_request=<n>", and both havebase.ref = feature/atomcompass_new. - L171 recommends stacking any dependent task on its unlanded parent. Under this definition, a second independent chain may not be linked.
The drift check on L179-181 is written in terms of "another open PR's branch", and that is the fork you mean (#63/#67/#75 on #59's branch). Fix, a few words on L178: "A fork (two open PRs based on one open PR's branch)". Optional, in the same edit: say whether an arm includes the PRs above it. #75 carries #91→#96. If #63 were the linked arm, #91 is based on an open PR's branch, and "fork arms excepted" does not clearly cover it.
There was a problem hiding this comment.
Fixed in the next commit: a fork is now "two or more open PRs based on one open PR's branch", so the integration branch no longer counts as a fork. I also dropped the repeated clause, as ponytail suggested.
There was a problem hiding this comment.
Resolved at c1e680189. The fork is now "two or more open PRs based on one open PR's branch". feature/atomcompass_new is not an open PR's branch, so stacks 138 and 134, and any further chain that L172 recommends, all link on it independently. The definition uses the same phrase as the drift check ("based on another open PR's branch"), so the two now agree.
I checked it against the live PR list. compass/ca-2-grant-rule (open #59) is the base of #63, #67 and #75, so it is a fork, as intended. compass/ir-1-data-model belongs to #60, which has merged, so #61 and #62 are no longer a fork. They are also outside the drift check. That is consistent: each can be linked as its own chain, and linking retargets it onto the integration branch, which the L105 clause now names. The optional "does an arm include the PRs above it" is still open. It is not blocking; see the new inline on L180.
| issue. The label stops all agent action on that issue or PR (no commit, review, | ||
| amend or merge, even after a passed review); only the owner removes it. | ||
| amend or merge, even after a passed review), with one exception: `gh stack | ||
| link` by PR number, which lands and pushes nothing. Only the owner removes it. |
There was a problem hiding this comment.
Not blocking — "lands and pushes nothing" is true, but linking also rewrites the base of each listed PR. The exception's reason does not say so.
Checked in the gh stack v0.1.1 source (cmd/link.go at tag v0.1.1, via gh api repos/github/gh-stack/contents/cmd/link.go?ref=v0.1.1):
- Pushes nothing: confirmed.
pushBranchArgspushes only arguments for whichgit.BranchExists(arg)is true.findExistingPRresolves a numeric argument as a PR first. No local branch in this clone has a purely numeric name, so linking by PR number pushes nothing. - Edits bases: not stated. After resolving,
runLinkCreateOrUpdatecallsfixBaseBranches. It sets the bottom PR's base to--baseand each later PR's base to the previous PR's head branch, and it callsUpdatePRBasewherever they differ. The L176 command over the CA tree (--base feature/atomcompass_new 59 75 91 96) therefore retargets held compass: the grant rule and the clock state machine (CA-2, #45) #59 fromcompass/ca-1-clock-identitytofeature/atomcompass_new. compass: logical-process identity, order and lookahead for the clock (CA-1, #44) #52 landed squashed on 2026-09-21, so compass: the grant rule and the clock state machine (CA-2, #45) #59's diff then also shows compass: logical-process identity, order and lookahead for the clock (CA-1, #44) #52's original commits. That is a visible change to a held PR, and nothing gets pushed to fix it. The same applies to compass(ir): find the nested repetition in a flat block sequence #62, which is still based on the landedcompass/ir-1-data-model. - Round 1 also asked the exception to name the retarget that a landing below a linked held member causes (L183). That half was not taken.
The exception names the command, so the text still licenses both edits. There is no contradiction, and that is why this does not block. One clause would make the reason complete: "…which lands and pushes nothing (it may retarget the held PR's base, as may a landing below it)".
There was a problem hiding this comment.
Added to the exception: "though it retargets the linked PRs' bases (then and when a PR below lands)".
There was a problem hiding this comment.
Resolved at c1e680189. L105-106 now says linking "retargets the linked PRs' bases (then and when a PR below lands)". This matches gh stack v0.1.1 cmd/link.go: runLinkCreateOrUpdate and runLinkAdd both call fixBaseBranches, which calls UpdatePRBase wherever a listed PR's base differs from the chain (the bottom PR goes to --base, and each later PR goes to the previous PR's head). The second half is the retarget that merge does to the PR above, which round 1 asked the exception to name. "Pushes nothing" still holds for PR-number arguments (pushBranchArgs is gated on git.BranchExists).
|
Agent-authored review, round 2 (delta Verdict: CHANGES REQUESTED — L178 defines a fork as "two open PRs on one base". That makes Nothing was pushed, amended, merged, labelled, linked or retargeted. I read the principles and Scope in Round-1 findings
New findings (inline)A. Blocking, L178. "Two open PRs on one base" includes the integration branch: #81, #85, #266, #292, #312, #314 and #315 are all based on it. Stacks 138 (#81→…→#252) and 134 (#85→…→#119) are both linked with Other items checked, no finding
ponytail-review
A is a one-phrase fix. With it in, I expect to approve on a delta review. |
…t linking does Answers the round-2 review of a8377a4: - "Two open PRs on one base" also matched the integration branch, which would have allowed only one stack there. A fork is now two or more open PRs based on one open PR's branch. - The link exception now says linking retargets the linked PRs' bases, both when linked and when a PR below lands. - Dropped a clause that repeated "links at most one arm". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| refuse): `gh stack link --base feature/atomcompass_new <bottom-pr#> ... <top-pr#>`, | ||
| re-run whenever a PR joins, held members included (linking lands nothing). | ||
| `merge` retargets only linked members. A fork (two or more open PRs based on | ||
| one open PR's branch) links at most one arm. Drift check: every open PR based |
There was a problem hiding this comment.
Not blocking. "Arm" is still undefined, and the live CA fork has an arm with PRs above it.
The new definition is right, and it no longer matches the integration branch (details in the round-3 summary). One ambiguity from round 2's optional note is left. compass/ca-2-grant-rule (#59's branch) is the base of #63, #67 and #75, and #75 carries #91→#96. If an arm means only the PR on the fork's branch, "fork arms excepted" covers #63, #67 and #75. It does not cover #91 or #96, which are "based on another open PR's branch" and so fall under the drift check. When #63 is the arm that gets linked, #91 and #96 then show as drift, and the only way to link them (link --base feature/atomcompass_new 75 91 96) would retarget #75 off #59's branch.
L176's "link a chain whole" already points to the natural choice, 59 75 91 96, and there the question never comes up, so this does not block. Optional fix, a few words: "links at most one arm (a PR on that branch and every PR above it)".
|
Agent-authored review, round 3 (delta Verdict: APPROVE. Covers head Nothing was pushed, amended, merged, labelled, linked or retargeted. I read the reviewer preamble and the eight principles in Round-2 findings
Regressions: noneRound 1: the label exception names New finding
For the landerThe base has moved to ponytail-review
|
Removes the rule against
gh stack linkon a chain that hasneed humananywhere below it. The owner asked for this after the #305 investigation.need humanon it or anywhere below it, so a held chain cannot merge through a link. The ban's only reason was that the label stops all agent action on its PR. The owner has now ruled that linking is allowed anyway.Second change: answer review findings with new commits, never an amend. Of the 14 force-pushes on open PRs since the no-force-push rule was added (#36, 2026-09-21), at least 10 broke it: #59, #62 (×3), #91, #95, #96 (×2), #120 and #252. Almost all of them amended commits to answer review findings. Branches land squashed, so an amend gains nothing. It costs two things:
<approved>..<head>is impossible.The rule now says to use new commits. The permitted rebase after a parent lands is unchanged.
🤖 Generated with Claude Code