gr verbs report success bound to control flow rather than observed destination state — eight open instances, plus the missing one in `gr push`

[laynepenney]

Eight open issues in this repo describe the same defect in eight different verbs. This one proposes treating it as a single architectural defect in how gr reports outcomes, and adds the member that is missing and most consequential: gr push.

The existing instances

Issue	Verb	Reports success while
#804	gr sync --repo <unknown>	matching zero repos
#836	gr branch --repo <unmatched>	creating nothing (exits 0)
#839	gr branch --repo <unmatched>	"All repos now on branch: X" having moved zero
#814	gr rebase <branch>	targeting the stale LOCAL ref, no-op
#768	gr rebase --abort	leaving the repo in state=rebasing
#873	gr sync	gripspace resolution degraded to a warning
#883	link --apply	composing content that is behind upstream
#809	gh project item-add (adjacent tooling)	exiting 0 on a silent no-op

Filed separately across three weeks, each by whoever hit it. Read together they are not eight bugs.

The missing member: gr push

Two measurements, two different symptoms, same day.

Symptom A — reported success, remote did not move. A push was reported as successful; the remote ref stayed at its prior SHA. Caught only because the author independently ran git ls-remote over HTTPS afterward, then re-pushed over HTTPS, which landed.

Symptom B — non-zero failures, zero exit code. Measured on a separate run:

⚠ 1 pushed, 1 failed, 17 skipped

  ✗ <repo>: Error: Operation failed: fatal: could not read Username for '<host>': Device not configured

$ echo $?
0

The command printed its own failure and exited 0. Any wrapper, script, or agent gating on the exit code reads that as success.

Why push is the member that matters most

Every other instance in the table wastes time. This one loses work. A push that reports success without moving the remote produces a local branch its author believes is published — so it is not re-pushed, not treated as at-risk, and not caught by any sweep that trusts the tool. The failure is silent at exactly the moment the safety net is supposed to engage.

It also defeats the standard remedy. The natural verification is to read back the remote-tracking ref, and remote-tracking refs are a local cache — they can be stale for reasons unrelated to the push. So a post-push check against origin/<branch> can agree with a push that never happened, and both instruments are wrong in the same direction.

The shape underneath

Every instance is the same substitution: an outcome signal bound to something adjacent to the outcome. The verb reports that it ran, that it iterated, that it encountered no exception in its own frame — never that the intended state change is now true at the destination.

That is why fixing them one at a time has not converged. Eight fixes in eight verbs leave the ninth verb free to make the same substitution, because nothing in the design says what a success claim is required to be about.

Proposed contract

A gr verb's success claim binds to observed destination state, not to internal control flow.

1. Match count is part of the result. Any verb taking a selector reports how many repos it matched. Zero matched is never silent, and never success. (gr sync --repo <unknown-name> reports success after matching zero repos #804, gr branch reports success and exits 0 when --repo matches no repos, creating nothing #836, gr branch --repo <unmatched> reports "All repos now on branch: X" having moved zero repos #839)
2. Mutating verbs re-read the thing they changed and report the observed value — remote SHA for push, current branch for branch, repo state for rebase --abort. (gr rebase --abort returns success but leaves repo in state=rebasing (cannot clear its own rebase metadata) #768, gr rebase <branch> targets the stale LOCAL ref and reports success on a no-op #814)
3. The exit code is the disjunction of per-repo results. Any repo failing makes the process exit non-zero. Printing a failure and exiting 0 is the defect in its purest form. (Symptom B)
4. Degradations are failures unless explicitly opted into. A resolution that falls back to a warning does not report success. (gripspace resolution failure degrades to a warning while gr sync reports success #873, gitgrip link --apply composes from whatever a gripspace already has on disk and reports full success even when that content is behind upstream #883)
5. Remote verification does not use remote-tracking refs. push confirms against the remote itself, because the cache can agree with a push that did not happen.

Suggested disposition

Close the eight as children of this one if the contract is adopted, or keep them open and land the contract incrementally with each closing under it. Either is fine. What does not work is a ninth instance issue.

Cross-cutting, so the natural home is the verb-result layer rather than any single command.

[laynepenney]

Rule 1 needs amending, not enforcing — and the discriminator that makes the rules applicable

Two things, one of which changes the contract.

The discriminator

The five rules are individually clear and collectively hard to apply at a call site. This test separates a real observation from a check that merely looks like one:

A guard on a flag the operation set about itself is control flow wearing the shape of an observation.

Ask of any success path: did the check consult something the operation could not have set, or did it read the operation's own self-report? Only the first is an observation. The second passes for the same reason the bug exists.

It is worth running the test on pr merge's own two checks, because they land on opposite sides:

• validate_repo_filters_known consults the manifest — something the operation could not have set. It passes the discriminator. It is a correct guard that answers a different question than "did anything match."
• The zero-match branch tests prs_to_merge.is_empty() — a flag the operation populated about itself, one arm of its own loop. It fails the discriminator, and that is exactly where the live defect sits.

So the discriminator is not a separate observation from the defect below. It is what locates it.

Rule 1 and the shipped behavior contradict each other

Rule 1 says a zero match is never silent and never success. pr merge says a well-formed request over an empty world is success, deliberately, with a test named for that intent.

Both are defensible. They cannot both be the contract.

The conflict survived because the rule lives in an issue and the behavior lives in a test, and neither surface has a reader that checks the other. Nothing anywhere went red.

The resolution is to amend rule 1, because the shipped line is the better one. Rule 1 collapses three quantities that do not nest:

Denominator	What it counts
Selection	repos the request resolved to — the quantity printed as "Repositories checked"
Actionable	of the selection, those found to have work to do
Errors	of the selection, those that errored — whether determining state or acting on it

Errors is deliberately not a subset of Actionable. A repo whose PR lookup fails never becomes actionable at all: it is inside the selection and disjoint from the actionable set, while a failure during the merge itself happens to something already inside it. Both are errors, and they sit at different depths. Any rule phrased as a chain of subsets will lose one of them, which is the specific way "zero matched" goes wrong.

A known repo with no open PR is a valid selection with zero actionable work — not a zero match — and reporting checked: 1 for it is honest.

The proposed partition, which the current code already implements in two of three cases:

1. Bad request — a name unknown to the manifest. Process-level refusal, distinct nonzero exit.
2. Valid selection, zero actionable results, zero errors. Success, with counts that name which denominator they report.
3. Any error, including mixed runs. Nonzero. One failure among four successes is still nonzero.

Case 3 is where the live defect sits. When every repo's PR lookup errors, each failure is printed, and then the summary reports the absence of work rather than the failure to determine it — and the process exits zero. Two printed failures and a success in one run.

Why rules 1 and 2 are both vulnerable to this

A rule written as a conjunction can be half-satisfied in a way that reads as compliance, and the half more likely to be checked is the one with a visible artifact. When a count is printed and a return value is not, the printed half gets the attention. Rule 2 has the same shape: re-reading the destination and reporting the observed value are independently fail-able, and a visible report can exist without the required re-read.

So an amended rule 1 must name its denominator explicitly. "Zero matched" is not a quantity until it says which of the three it means.

---

The partition above is Atlas's, from the design already shipped in pr merge. The discriminator and the case-3 evidence are Apollo's, the latter measured at the process boundary — the real binary run against the pre-fix code, with every PR lookup failing, asserting on its exit status. Earlier drafts of this comment attributed the defect to a path that no longer exists and to a case that turned out to be intended behavior, and a further draft nested these denominators as a chain of subsets; all three were caught in review before publication.