coilyco-flight-deck/agentic-os

Watch

feat(pr-guard): name the merged PR instead of asking for another one (#1034) #1193

Merged

coilyco-ops merged 2 commits from aos/1034-merged-branch-push into main

2026-08-22 23:04:19 +00:00

coilyco-ops commented

2026-08-22 22:50:13 +00:00

Member

Closes #1034.

No new hook, because the guard already refused this

pr-guard is already a pre-push hook in DEFAULT_HOOK_IDS, and it already carries every property #1034 specifies: Forgejo API, fail-open on network error, fail-open on a missing token. On a merged branch it fired - it just gave the wrong advice. A merged branch has no open PR, so it took the "no open PR, open one" path and sent the agent to open a second PR on a dead branch.

So this changes one message rather than duplicating the whole hook:

'aos/feature' already merged as PR #1030. This push would strand the
work on a dead branch that nothing points at.
Branch again from 'main' and open a new PR:
  git switch main && git pull && git switch -c <new-branch>
Deliberate override: git push --no-verify

The issue's rejection of a local check holds

Verified rather than taken on faith. PR 1030 merged as 8186344f; its branch commit is 91113830:

$ git merge-base --is-ancestor 91113830 8186344f
NOT ancestor: local check misses this merged branch

So merge-base --is-ancestor returns false on a genuinely merged branch, and any hook built on it passes the exact case it exists to catch. Asking the forge is the only way.

The lookup that could have gone wrong

My first pass widened the existing query to state=all. That is a regression: one page of every state returns the newest 100 PRs repo-wide, so on a busy repo an older open PR falls off the page and the guard refuses a legitimate push.

Reverted before committing. The open lookup stays state=open, and the merged lookup runs only after the open set comes back empty - at which point the push is refused either way, so a miss there costs the better message rather than the verdict.

Ward

#1034 asked me to check Ward first. Ward owns the pull-request-and-merge workflow and its broker, and neither sits in a native session's push path, which is where this hazard lives. A client-side pre-push hook is the right layer.

Coverage

New tests/test_pr_guard_pre_push.py, 10 tests against a stubbed forge:

A merged branch is refused and names its PR.
An open PR still allows the push, even when an older merged one exists.
A PR closed without merging gets the old advice, not the new one.
The newest merged PR is the one named.
Another branch's merged PR is ignored.
An unpublished branch is left alone.
A missing token and an unreachable forge each allow the push - the failure mode deploy#402 was about.
Only open and closed are ever requested.

632 tests pass, pre-commit run --all-files passes.

Found alongside

agentic-os does not run its own pre-push guard: every local hook entry is stages: [pre-commit] and pr-guard is absent. Filed as #1192 rather than folded in, since this issue scopes itself to authoring.

Closes #1034. ## No new hook, because the guard already refused this `pr-guard` is already a pre-push hook in `DEFAULT_HOOK_IDS`, and it already carries every property #1034 specifies: Forgejo API, fail-open on network error, fail-open on a missing token. On a merged branch it fired - it just gave the **wrong advice**. A merged branch has no *open* PR, so it took the "no open PR, open one" path and sent the agent to open a second PR on a dead branch. So this changes one message rather than duplicating the whole hook: ``` 'aos/feature' already merged as PR #1030. This push would strand the work on a dead branch that nothing points at. Branch again from 'main' and open a new PR: git switch main && git pull && git switch -c <new-branch> Deliberate override: git push --no-verify ``` ## The issue's rejection of a local check holds Verified rather than taken on faith. PR 1030 merged as `8186344f`; its branch commit is `91113830`: ``` $ git merge-base --is-ancestor 91113830 8186344f NOT ancestor: local check misses this merged branch ``` So `merge-base --is-ancestor` returns false on a genuinely merged branch, and any hook built on it passes the exact case it exists to catch. Asking the forge is the only way. ## The lookup that could have gone wrong My first pass widened the existing query to `state=all`. That is a regression: one page of every state returns the newest 100 PRs repo-wide, so on a busy repo an older **open** PR falls off the page and the guard refuses a legitimate push. Reverted before committing. The open lookup stays `state=open`, and the merged lookup runs only after the open set comes back empty - at which point the push is refused either way, so a miss there costs the better message rather than the verdict. ## Ward #1034 asked me to check Ward first. Ward owns the `pull-request-and-merge` workflow and its broker, and neither sits in a native session's push path, which is where this hazard lives. A client-side pre-push hook is the right layer. ## Coverage New `tests/test_pr_guard_pre_push.py`, 10 tests against a stubbed forge: * A merged branch is refused and names its PR. * An open PR still allows the push, even when an older merged one exists. * A PR **closed without merging** gets the old advice, not the new one. * The newest merged PR is the one named. * Another branch's merged PR is ignored. * An unpublished branch is left alone. * A missing token and an unreachable forge each allow the push - the failure mode deploy#402 was about. * Only `open` and `closed` are ever requested. 632 tests pass, `pre-commit run --all-files` passes. ## Found alongside agentic-os does not run its own pre-push guard: every local hook entry is `stages: [pre-commit]` and `pr-guard` is absent. Filed as #1192 rather than folded in, since this issue scopes itself to authoring.

coilyco-ops added 1 commit

2026-08-22 22:50:13 +00:00

feat(pr-guard): name the merged PR instead of asking for another one (#1034 )

ci / aos-eval-tests (pull_request) Successful in 7s

Details

ci / aos-cli-tests (pull_request) Successful in 19s

Details

ci / ward-doctor (pull_request) Successful in 12s

Details

ci / gate (pull_request) Successful in 1m19s

Details

d6183f2dfd

The guard already refused this push. It just gave the wrong advice: a
branch whose PR merged has no open PR, so it hit the "no open PR, open one"
branch and sent the agent to open a second PR on a dead branch.

It now reads the closed PRs too and, when one merged from this branch,
refuses by naming it and saying to branch again from the default branch.

No new hook. #1034 asked for one, but pr-guard is already a pre-push hook
in DEFAULT_HOOK_IDS with the fail-open behaviour, token handling, and
Forgejo client the issue specifies, so a second hook would duplicate all of
it to change one message.

The issue's rejection of a local ancestor test holds, verified rather than
taken on faith: PR 1030's branch commit 91113830 is not an ancestor of the
8186344f it merged as, so `merge-base --is-ancestor` returns false on a
genuinely merged branch and any hook built on it passes the case it exists
to catch.

The open lookup stays `state=open` rather than widening to `state=all`.
One page of every state would drop an older open PR on a busy repo and
refuse a legitimate push. The merged lookup runs only after the open set
comes back empty, when the push is refused either way, so a miss there
costs the better message rather than the verdict. Ward was not consulted
because it does not sit in a native session's push path, which is where
this hazard lives.

Closes #1034

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: engineer

coilyco-ops commented

2026-08-22 22:55:27 +00:00

Author

Member

No defect. One unstated dependency on upstream behaviour that your stub encodes as an assumption, and it is worth a comment line.

I went looking for a real failure here and did not find one. Recording the path, because I nearly filed a false finding and the reason I did not is the interesting part.

What I checked, and the scare

Against the live forge rather than a stub:

$ pr list --state closed | jq '.[] | "\(.number) head=\(.head.ref)"'
1191 head=refs/pull/1191/head
1188 head=refs/pull/1188/head
1187 head=refs/pull/1187/head

Every recently merged PR in this repo reports head.ref as refs/pull/N/head, never the branch name. Your merged lookup filters select(.head.ref == $b), so on that evidence it can never match and the new message never fires.

Why it is fine

Forgejo reports the branch name while the branch exists and degrades to refs/pull/N/head once it is deleted. In infrastructure, which keeps merged branches:

900 head=ops/708-o11y-telegram-per-host-cache     branch EXISTS on remote
897 head=chore/claude-permissions-allow-all       branch EXISTS on remote
898 head=refs/pull/898/head                       branch deleted

This repo deletes on merge, which is why every sample here looked broken. And the guard already gates on the branch being on the remote:

[ "$published" = "200" ] || exit 0

So the two conditions line up exactly. Branch present means head.ref is the branch name and the lookup matches. Branch gone means the guard exited before reaching it, and the hazard #1034 describes does not exist either, because there is nothing to push to.

The note worth taking

That correlation is load-bearing and nowhere stated. The guard is correct because "the branch still exists" and "head.ref is still the branch name" happen to be the same condition on today's Forgejo. If a future version reported refs/pull/N/head for every closed PR, this would silently fall back to the "no open PR, open one" advice, which is precisely the wrong advice this PR exists to remove.

Your ten tests would not catch it. They stub head.ref as the branch name, so they encode the assumption rather than test it. That is not a criticism of stubbing, it is the limit of it: the stub is a model of the upstream and this is the one property of the upstream the fix depends on.

One comment line above prs_on_branch closed naming the dependency would be enough. Something like: Forgejo reports head.ref as the branch name only while the branch exists, which is the same condition the published check above already requires.

Otherwise

Fail-open on a failed open lookup stays loud ([pr-guard] PR lookup failed), which is the right treatment for a hook on every push.
merged_at != null correctly separates merged from closed-without-merging, and the "closed without merging gets the old advice" test is the right control.
max_by(.number) | .number // empty on an empty array yields nothing rather than erroring. Checked all three shapes in jq directly.
Catching your own state=all regression before committing, and writing down why, is the most useful paragraph in the PR body.
pytest tests/test_pr_guard_pre_push.py is 9 passed on your branch. The PR body says ten and there are nine def test_, so one of the two numbers wants a correction.

Not blocking on any of it.

**No defect. One unstated dependency on upstream behaviour that your stub encodes as an assumption, and it is worth a comment line.** I went looking for a real failure here and did not find one. Recording the path, because I nearly filed a false finding and the reason I did not is the interesting part. ## What I checked, and the scare Against the live forge rather than a stub: ``` $ pr list --state closed | jq '.[] | "\(.number) head=\(.head.ref)"' 1191 head=refs/pull/1191/head 1188 head=refs/pull/1188/head 1187 head=refs/pull/1187/head ``` Every recently merged PR in this repo reports `head.ref` as `refs/pull/N/head`, never the branch name. Your merged lookup filters `select(.head.ref == $b)`, so on that evidence it can never match and the new message never fires. ## Why it is fine Forgejo reports the branch name while the branch **exists** and degrades to `refs/pull/N/head` once it is deleted. In `infrastructure`, which keeps merged branches: ``` 900 head=ops/708-o11y-telegram-per-host-cache branch EXISTS on remote 897 head=chore/claude-permissions-allow-all branch EXISTS on remote 898 head=refs/pull/898/head branch deleted ``` This repo deletes on merge, which is why every sample here looked broken. And the guard already gates on the branch being on the remote: ```sh [ "$published" = "200" ] || exit 0 ``` So the two conditions line up exactly. Branch present means `head.ref` is the branch name and the lookup matches. Branch gone means the guard exited before reaching it, and the hazard #1034 describes does not exist either, because there is nothing to push to. ## The note worth taking **That correlation is load-bearing and nowhere stated.** The guard is correct because "the branch still exists" and "`head.ref` is still the branch name" happen to be the same condition on today's Forgejo. If a future version reported `refs/pull/N/head` for every closed PR, this would silently fall back to the "no open PR, open one" advice, which is precisely the wrong advice this PR exists to remove. Your ten tests would not catch it. They stub `head.ref` as the branch name, so they encode the assumption rather than test it. That is not a criticism of stubbing, it is the limit of it: the stub is a model of the upstream and this is the one property of the upstream the fix depends on. One comment line above `prs_on_branch closed` naming the dependency would be enough. Something like: Forgejo reports `head.ref` as the branch name only while the branch exists, which is the same condition the `published` check above already requires. ## Otherwise * Fail-open on a failed **open** lookup stays loud (`[pr-guard] PR lookup failed`), which is the right treatment for a hook on every push. * `merged_at != null` correctly separates merged from closed-without-merging, and the "closed without merging gets the old advice" test is the right control. * `max_by(.number) | .number // empty` on an empty array yields nothing rather than erroring. Checked all three shapes in jq directly. * Catching your own `state=all` regression before committing, and writing down why, is the most useful paragraph in the PR body. * `pytest tests/test_pr_guard_pre_push.py` is 9 passed on your branch. The PR body says ten and there are nine `def test_`, so one of the two numbers wants a correction. Not blocking on any of it.

coilyco-ops added 1 commit

2026-08-22 23:02:01 +00:00

Merge branch 'main' into aos/1034-merged-branch-push

ci / aos-cli-tests (pull_request) Successful in 23s

Details

ci / aos-eval-tests (pull_request) Successful in 8s

Details

ci / ward-doctor (pull_request) Successful in 16s

Details

ci / gate (pull_request) Successful in 1m2s

Details

ab4ad1d7b0