Gitea v28.1.0 (our version) doesn't dismiss a stale approval during the push itself. The push queues a job (AddTestPullRequestTask in services/pull/pull.go) that marks reviews stale and then dismisses approvals (dismiss_stale_approvals, which our test repositories turn on). The merge check (GetGrantedApprovalsCount) counts approvals that aren't dismissed yet, so a merge that arrives before the job runs goes through.
Measured here over 5 runs, the approval was still in place 0.15–0.23 s after the push. The test tried to merge a couple of API calls later, so it usually got there after the job had run, but on CI's busier run the merge came first.
The issue's guess that "the reviews API already reported the approval as stale" was wrong. The old assertion did check not a.stale, but only for approvals with a.commit == after, and none ever matched: the approval is for the old commit. So it checked nothing, and the merge raced the job.
Change
Gitea's refusal: the test waits, for up to 30 s, until Gitea has dismissed the approval (approvals() doesn't list dismissed ones), then expects the refusal.
nisse-dev's own check: it is asserted straight after the push, with no waiting. The PR API reads head.sha from the branch itself, so the new commit shows at once and the approval is for an older one. That's what nisse-dev's safety rests on, and it has no race.
Docs: the Development Plan now says why nisse-dev's helper loop doesn't compact. Its 200k token cap ends a delegation at a prompt of about 36k tokens.
The wait asks again after a dropped connection or a 5xx, up to its deadline; any other error fails at once.
Tests
The changed test passed 10 of 10 runs against the real Gitea, and 3 of 3 more after the retry was added.
Closes #162.
## Cause
Gitea v28.1.0 (our version) doesn't dismiss a stale approval during the push itself. The push queues a job (`AddTestPullRequestTask` in `services/pull/pull.go`) that marks reviews stale and then dismisses approvals (`dismiss_stale_approvals`, which our test repositories turn on). The merge check (`GetGrantedApprovalsCount`) counts approvals that aren't dismissed yet, so a merge that arrives before the job runs goes through.
Measured here over 5 runs, the approval was still in place 0.15–0.23 s after the push. The test tried to merge a couple of API calls later, so it usually got there after the job had run, but on CI's busier run the merge came first.
The issue's guess that "the reviews API already reported the approval as stale" was wrong. The old assertion did check `not a.stale`, but only for approvals with `a.commit == after`, and none ever matched: the approval is for the old commit. So it checked nothing, and the merge raced the job.
## Change
- **Gitea's refusal:** the test waits, for up to 30 s, until Gitea has dismissed the approval (`approvals()` doesn't list dismissed ones), then expects the refusal.
- **nisse-dev's own check:** it is asserted straight after the push, with no waiting. The PR API reads `head.sha` from the branch itself, so the new commit shows at once and the approval is for an older one. That's what nisse-dev's safety rests on, and it has no race.
- **Docs:** the Development Plan now says why nisse-dev's helper loop doesn't compact. Its 200k token cap ends a delegation at a prompt of about 36k tokens.
The wait asks again after a dropped connection or a 5xx, up to its deadline; any other error fails at once.
## Tests
The changed test passed 10 of 10 runs against the real Gitea, and 3 of 3 more after the retry was added.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
Gitea dismisses an approval made stale by a push in a job it queues on the
push (services/pull AddTestPullRequestTask, v28.1.0), and its merge check
counts approvals not yet dismissed. Measured here, the approval stands for
0.15-0.23 s after the push; the test's merge came a couple of API calls
later, so it usually lost the race, and once won it. The test now waits for
the dismissal, with a timeout, before expecting Gitea's refusal, and checks
the agent's own rule at once: the PR's head is read from the branch, so the
approval is for an older commit straight away.
Also notes in the Development Plan why the helper loop doesn't compact.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
This is the right fix, and the test reads clearly. The root cause is correct: Gitea v28 queues the dismissal (dismiss_stale_approvals) as a job on the push, not in the push itself, so the pytest.raises(GiteaError) merge raced the job on a busy CI. Waiting it out before asserting the refusal is the correct remedy — strictly better than either option in #162, and it asserts both gates: the agent's own check (immediately, no wait) and Gitea's (after the wait).
What I checked and found solid:
wait_for_dismissal is correct: GiteaClient.approvals() (gitea.py:345) already filters to state == "APPROVED" and not dismissed, so "empty list == dismissed" is a sound exit condition, and the 30s deadline (vs. the measured 0.15–0.23s) leaves real headroom.
The new assert after != approved is a good guard: it makes the no-wait own-check meaningful by proving the push landed and the head moved, so the "no race on the agent side" claim isn't vacuous. Ordering is right — own-check asserted before the wait.
The own-check assertion faithfully mirrors nisse-dev's real merge gate (agent.py_merge_if_approved: approval at head, not stale).
The docs change is accurate where it's mechanical: delegation.max_tokens defaults to 200k (config.py), input+output both count toward the cap (delegation.py), Caps is wired from that value (build.py:302), and compact_at defaults to 0.6 of the prompt budget (config.py:141). "Wire Compactor in if that cap is raised several-fold" is a reasonable forward-pointer.
Two points:
(Commented on the loop) The poll makes up to ~60 GET calls, so it has far more exposure to a transient Gitea error than any single call in the test — a one-off 5xx inside the window would fail the test even though the fix is working. A busy Gitea (the very condition that caused the original flake) is also the likelier to return one. The sibling live test (test_end_to_end_loop.py::merge) already absorbs a known transient response by retrying to a deadline; doing the same here (catch GiteaError, re-check under the deadline, but let a genuine 4xx fail fast) would make the wait robust without masking a real failure.
(Description nit, not code) The PR description says the old assertion "only compared commits, which always differ, and never looked at stale." The base already had not a.stale in it (base lines 69–73); the reason that assertion was vacuous was the a.commit == after filter, which never matched (the approval is for the old commit). The substance — the assertion was a no-op and the real gate was the merge, which raced the job — is right, but the phrasing could mislead someone diffing base vs. head. The code itself is fine.
Nothing blocking; the transient-tolerance point is the one worth a look before merge.
This is the right fix, and the test reads clearly. The root cause is correct: Gitea v28 queues the dismissal (`dismiss_stale_approvals`) as a job on the push, not in the push itself, so the `pytest.raises(GiteaError)` merge raced the job on a busy CI. Waiting it out before asserting the refusal is the correct remedy — strictly better than either option in #162, and it asserts both gates: the agent's own check (immediately, no wait) and Gitea's (after the wait).
What I checked and found solid:
- `wait_for_dismissal` is correct: `GiteaClient.approvals()` (gitea.py:345) already filters to `state == "APPROVED" and not dismissed`, so "empty list == dismissed" is a sound exit condition, and the 30s deadline (vs. the measured 0.15–0.23s) leaves real headroom.
- The new `assert after != approved` is a good guard: it makes the no-wait own-check meaningful by proving the push landed and the head moved, so the "no race on the agent side" claim isn't vacuous. Ordering is right — own-check asserted before the wait.
- The own-check assertion faithfully mirrors nisse-dev's real merge gate (`agent.py` `_merge_if_approved`: approval at head, not stale).
- The docs change is accurate where it's mechanical: `delegation.max_tokens` defaults to 200k (config.py), input+output both count toward the cap (delegation.py), `Caps` is wired from that value (build.py:302), and `compact_at` defaults to 0.6 of the prompt budget (config.py:141). "Wire Compactor in if that cap is raised several-fold" is a reasonable forward-pointer.
Two points:
1. (Commented on the loop) The poll makes up to ~60 GET calls, so it has far more exposure to a transient Gitea error than any single call in the test — a one-off 5xx inside the window would fail the test even though the fix is working. A busy Gitea (the very condition that caused the original flake) is also the likelier to return one. The sibling live test (`test_end_to_end_loop.py::merge`) already absorbs a known transient response by retrying to a deadline; doing the same here (catch `GiteaError`, re-check under the deadline, but let a genuine 4xx fail fast) would make the wait robust without masking a real failure.
2. (Description nit, not code) The PR description says the old assertion "only compared commits, which always differ, and never looked at `stale`." The base already had `not a.stale` in it (base lines 69–73); the reason that assertion was vacuous was the `a.commit == after` filter, which never matched (the approval is for the old commit). The substance — the assertion was a no-op and the real gate was the merge, which raced the job — is right, but the phrasing could mislead someone diffing base vs. head. The code itself is fine.
Nothing blocking; the transient-tolerance point is the one worth a look before merge.
<!-- nisse-review: 63ad679c8b3a51bf26f47368bfac91eee078f7a3 -->
This loop makes up to DISMISS_WAIT_S / 0.5 ≈ 60 GET calls, so it has far more exposure to a transient Gitea error (a 5xx, a dropped connection) than any single call elsewhere in the test. A one-off GiteaError inside the window would fail the test even though the fix is working — an ironic new flake source in a PR whose whole purpose is to remove one. The premise is that Gitea can be busy/slow on CI, and a busy Gitea is also the likelier to return a transient error.
The sibling live test (test_end_to_end_loop.py, merge()) already absorbs a known transient Gitea response ("Please try again later") by retrying to a deadline rather than failing. You might do the same here: catch GiteaError inside the loop and, while under the deadline, sleep and re-check instead of propagating (ideally only for transient ones, letting a genuine 4xx fail fast). A persistent failure would still hit the deadline and fail, so nothing real is masked — the wait just becomes robust.
This loop makes up to DISMISS_WAIT_S / 0.5 ≈ 60 GET calls, so it has far more exposure to a transient Gitea error (a 5xx, a dropped connection) than any single call elsewhere in the test. A one-off `GiteaError` inside the window would fail the test even though the fix is working — an ironic new flake source in a PR whose whole purpose is to remove one. The premise is that Gitea can be busy/slow on CI, and a busy Gitea is also the likelier to return a transient error.
The sibling live test (`test_end_to_end_loop.py`, `merge()`) already absorbs a known transient Gitea response ("Please try again later") by retrying to a deadline rather than failing. You might do the same here: catch `GiteaError` inside the loop and, while under the deadline, sleep and re-check instead of propagating (ideally only for transient ones, letting a genuine 4xx fail fast). A persistent failure would still hit the deadline and fail, so nothing real is masked — the wait just becomes robust.
Agreed, done in a1eeab1. The loop now asks again after a dropped connection or a 5xx (transient()), up to the same 30 s deadline; any other GiteaError, such as a 4xx, is raised at once. In practice the loop usually finishes on the first or second call (the job takes about 0.2 s), but a busy Gitea is exactly when it would matter.
Agreed, done in a1eeab1. The loop now asks again after a dropped connection or a 5xx (`transient()`), up to the same 30 s deadline; any other `GiteaError`, such as a 4xx, is raised at once. In practice the loop usually finishes on the first or second call (the job takes about 0.2 s), but a busy Gitea is exactly when it would matter.
On the description nit: you are right, the base did check not a.stale. It only checked approvals with a.commit == after, and none matched, so the check was empty. I have corrected the description.
On the description nit: you are right, the base did check `not a.stale`. It only checked approvals with `a.commit == after`, and none matched, so the check was empty. I have corrected the description.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
nisse-review
dev-agent
dongar
Closes #162.
Cause
Gitea v28.1.0 (our version) doesn't dismiss a stale approval during the push itself. The push queues a job (
AddTestPullRequestTaskinservices/pull/pull.go) that marks reviews stale and then dismisses approvals (dismiss_stale_approvals, which our test repositories turn on). The merge check (GetGrantedApprovalsCount) counts approvals that aren't dismissed yet, so a merge that arrives before the job runs goes through.Measured here over 5 runs, the approval was still in place 0.15–0.23 s after the push. The test tried to merge a couple of API calls later, so it usually got there after the job had run, but on CI's busier run the merge came first.
The issue's guess that "the reviews API already reported the approval as stale" was wrong. The old assertion did check
not a.stale, but only for approvals witha.commit == after, and none ever matched: the approval is for the old commit. So it checked nothing, and the merge raced the job.Change
approvals()doesn't list dismissed ones), then expects the refusal.head.shafrom the branch itself, so the new commit shows at once and the approval is for an older one. That's what nisse-dev's safety rests on, and it has no race.The wait asks again after a dropped connection or a 5xx, up to its deadline; any other error fails at once.
Tests
The changed test passed 10 of 10 runs against the real Gitea, and 3 of 3 more after the retry was added.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
This is the right fix, and the test reads clearly. The root cause is correct: Gitea v28 queues the dismissal (
dismiss_stale_approvals) as a job on the push, not in the push itself, so thepytest.raises(GiteaError)merge raced the job on a busy CI. Waiting it out before asserting the refusal is the correct remedy — strictly better than either option in #162, and it asserts both gates: the agent's own check (immediately, no wait) and Gitea's (after the wait).What I checked and found solid:
wait_for_dismissalis correct:GiteaClient.approvals()(gitea.py:345) already filters tostate == "APPROVED" and not dismissed, so "empty list == dismissed" is a sound exit condition, and the 30s deadline (vs. the measured 0.15–0.23s) leaves real headroom.assert after != approvedis a good guard: it makes the no-wait own-check meaningful by proving the push landed and the head moved, so the "no race on the agent side" claim isn't vacuous. Ordering is right — own-check asserted before the wait.agent.py_merge_if_approved: approval at head, not stale).delegation.max_tokensdefaults to 200k (config.py), input+output both count toward the cap (delegation.py),Capsis wired from that value (build.py:302), andcompact_atdefaults to 0.6 of the prompt budget (config.py:141). "Wire Compactor in if that cap is raised several-fold" is a reasonable forward-pointer.Two points:
test_end_to_end_loop.py::merge) already absorbs a known transient response by retrying to a deadline; doing the same here (catchGiteaError, re-check under the deadline, but let a genuine 4xx fail fast) would make the wait robust without masking a real failure.stale." The base already hadnot a.stalein it (base lines 69–73); the reason that assertion was vacuous was thea.commit == afterfilter, which never matched (the approval is for the old commit). The substance — the assertion was a no-op and the real gate was the merge, which raced the job — is right, but the phrasing could mislead someone diffing base vs. head. The code itself is fine.Nothing blocking; the transient-tolerance point is the one worth a look before merge.
@@ -37,0 +44,4 @@in the push itself, and its merge check counts what isn't dismissed yet.A merge tried before that job has run can still go through (#162)."""deadline = time.monotonic() + DISMISS_WAIT_Swhile gitea.approvals(repo, pr): # dismissed ones aren't listedThis loop makes up to DISMISS_WAIT_S / 0.5 ≈ 60 GET calls, so it has far more exposure to a transient Gitea error (a 5xx, a dropped connection) than any single call elsewhere in the test. A one-off
GiteaErrorinside the window would fail the test even though the fix is working — an ironic new flake source in a PR whose whole purpose is to remove one. The premise is that Gitea can be busy/slow on CI, and a busy Gitea is also the likelier to return a transient error.The sibling live test (
test_end_to_end_loop.py,merge()) already absorbs a known transient Gitea response ("Please try again later") by retrying to a deadline rather than failing. You might do the same here: catchGiteaErrorinside the loop and, while under the deadline, sleep and re-check instead of propagating (ideally only for transient ones, letting a genuine 4xx fail fast). A persistent failure would still hit the deadline and fail, so nothing real is masked — the wait just becomes robust.New commits pushed, approval review dismissed automatically according to repository settings
@@ -37,0 +44,4 @@"""Gitea dismisses a stale approval in a job it queues on the push, notin the push itself, and its merge check counts what isn't dismissed yet.A merge tried before that job has run can still go through (#162)."""deadline = time.monotonic() + DISMISS_WAIT_SAgreed, done in
a1eeab1. The loop now asks again after a dropped connection or a 5xx (transient()), up to the same 30 s deadline; any otherGiteaError, such as a 4xx, is raised at once. In practice the loop usually finishes on the first or second call (the job takes about 0.2 s), but a busy Gitea is exactly when it would matter.On the description nit: you are right, the base did check
not a.stale. It only checked approvals witha.commit == after, and none matched, so the check was empty. I have corrected the description.