Trust test: wait for Gitea to dismiss a stale approval before merging (#162) #164

Merged
dev-agent merged 2 commits from flaky-stale-approval into main 2026-10-18 13:23:25 +00:00
2 changed files with 30 additions and 3 deletions
Showing only changes of commit 63ad679c8b - Show all commits

No files matched your search

+5 -2
View File
@@ -782,8 +782,11 @@ if the implementation differs.
with code's list of the calls they replace, become a `compaction` event with code's list of the calls they replace, become a `compaction` event
that renders in place of its range; a later one folds in the earlier. that renders in place of its range; a later one folds in the earlier.
Failed notes are logged (`context.compaction_failed`) and fitting takes Failed notes are logged (`context.compaction_failed`) and fitting takes
over; `context.compacted` logs each one. Not yet in nisse-dev's helper over; `context.compacted` logs each one. Not in nisse-dev's helper loop,
loop. on purpose (operator, 2026-10-18): its token cap (`delegation.max_tokens`,
200k, every call's whole prompt counted) ends a delegation at a prompt of
about 36k tokens, well short of `compact_at` on any window in use. Wire
`Compactor` in if that cap is raised several-fold.
- **L6 fitting order:** first shorten old tool results to their first and last - **L6 fitting order:** first shorten old tool results to their first and last
lines, then drop the oldest events with a note saying so; never the system lines, then drop the oldest events with a note saying so; never the system
prompt, the current plan or the most recent turns. View labels: prompt, the current plan or the most recent turns. View labels:
+25 -1
View File
@@ -1,9 +1,11 @@
"""The trust dial's hard wall, on a real Gitea (D4, D5; M3.3): with the """The trust dial's hard wall, on a real Gitea (D4, D5; M3.3): with the
protection a `merge: approved` project has, the agent's own token can't protection a `merge: approved` project has, the agent's own token can't
merge its PR until an operator approves the PR as it is now; it can then; merge its PR until an operator approves the PR as it is now; it can then;
and a commit pushed after the approval needs approving again.""" and a commit pushed after the approval needs approving again: at once by the
agent's own check, and by Gitea once it has dismissed the approval."""
import base64 import base64
import time
import uuid import uuid
from pathlib import Path from pathlib import Path
@@ -15,6 +17,9 @@ from nisse_dev.config import AgentConfig
pytestmark = pytest.mark.integration pytestmark = pytest.mark.integration
# How long Gitea gets to dismiss an approval after a push (#162).
DISMISS_WAIT_S = 30.0
def add_file(gitea: GiteaClient, repo: str, branch: str, path: str) -> None: def add_file(gitea: GiteaClient, repo: str, branch: str, path: str) -> None:
gitea._request( gitea._request(
@@ -34,6 +39,18 @@ def approve(operator: GiteaClient, repo: str, pr: int) -> None:
) )
def wait_for_dismissal(gitea: GiteaClient, repo: str, pr: int) -> None:
"""Gitea dismisses a stale approval in a job it queues on the push, not
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_S
while gitea.approvals(repo, pr): # dismissed ones aren't listed
Review

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.
Review

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.
assert time.monotonic() < deadline, (
f"Gitea didn't dismiss the approval within {DISMISS_WAIT_S}s"
)
time.sleep(0.5)
def test_merging_needs_the_operators_approval_of_the_pr_as_it_is( def test_merging_needs_the_operators_approval_of_the_pr_as_it_is(
tmp_path: Path, live_config: AgentConfig, gitea: GiteaClient tmp_path: Path, live_config: AgentConfig, gitea: GiteaClient
) -> None: ) -> None:
@@ -64,12 +81,19 @@ def test_merging_needs_the_operators_approval_of_the_pr_as_it_is(
name, head="m-2", base="main", title="b", body="" name, head="m-2", base="main", title="b", body=""
) )
approve(operator, name, second.number) approve(operator, name, second.number)
approved = gitea.pull_request(name, second.number).head_sha
add_file(gitea, name, "m-2", "c.txt") # a fix pushed after the approval add_file(gitea, name, "m-2", "c.txt") # a fix pushed after the approval
# At once, with no waiting: the PR's head is read from the branch,
# so the agent sees the new commit, and the approval is for the old
# one. Its own check (an approval at the head, not stale) holds off.
after = gitea.pull_request(name, second.number).head_sha after = gitea.pull_request(name, second.number).head_sha
assert after != approved
assert not [ assert not [
a a
for a in gitea.approvals(name, second.number) for a in gitea.approvals(name, second.number)
if a.commit == after and not a.stale if a.commit == after and not a.stale
] ]
# Gitea's own refusal comes once it has dismissed the approval.
wait_for_dismissal(gitea, name, second.number)
with pytest.raises(GiteaError): with pytest.raises(GiteaError):
gitea.merge_pull_request(name, second.number) gitea.merge_pull_request(name, second.number)