pbwraith/gitea-saas-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Gitea client: production review-policy client operations (#28) #31

Merged
pbwraith merged 3 commits from issue-28 into main 2026-08-29 09:51:20 +02:00
Conversation 5 Commits 3 Files Changed 2 +880 -18
pbwraith commented 2026-08-28 20:40:15 +02:00
Owner
Copy Link

Closes #28

Implements the production review-policy protocol on GiteaClient, plus hermetic tests.

What landed

gitea_saas/gitea_client.py:

  • list_open_prs(repo) — all and only open PRs (state=open), follows Gitea pagination (page/limit, 50/page, defensive 100-page cap), normalized shape.
  • add_label(repo, issue_number, label) / remove_label(repo, issue_number, label) — resolve the repo label ID, POST {"labels": [id]} / DELETE /labels/{id}, then verify the resulting label state via a re-GET. Idempotent no-op when already present/absent. Unknown label or failed verification raises GiteaClientError (never silently skipped).
  • merge_pr(repo, pr_number) -> bool — Gitea merge endpoint; already-merged PR short-circuits to True; after the merge request the PR is re-fetched and merged confirmed. Non-2xx (e.g. 409) raises.
  • close_issue(repo, issue_number) — PATCH state: closed and confirm the issue reports closed.
  • get_pr() now returns the normalized candidate shape: {number, body, head_sha, base_ref, labels (names), repo, changed_files} (changed_files kept for the review gate).
  • All non-2xx responses raise GiteaApiError with method/path/status/redacted body. Token and any echoed Authorization header are scrubbed from error text (defensively, even when a server echoes the request back).

tests/test_gitea_client.py (new) — local threaded HTTP stub on 127.0.0.1; asserts request methods, paths, bodies, pagination, normalized results, failure responses, no-token-leak, and a source-derived contract test that the production client exposes every operation ReviewPolicy/the reviewer call. No live Gitea contact.

Acceptance evidence

  1. Pagination + normalization: test_list_open_prs_follows_pagination_and_normalizes (50-item page 1 -> page 2 -> stop; state=open&limit=50 on every request).
  2. Label mutations resolve IDs, POST/DELETE correctly, verify state; unknown label / failed verification raise: 6 dedicated tests.
  3. merge_pr posts to /pulls/{n}/merge and confirms merged: 3 tests (incl. 409 raise, already-merged short-circuit).
  4. close_issue PATCHes {"state": "closed"} and confirms: 2 tests.
  5. Non-2xx useful error, token + Authorization header never leak (hostile echo-server test).
  6. Hermetic: 22 tests over the local stub, no network beyond 127.0.0.1.
  7. Contract: test_production_client_exposes_review_policy_contract.
  8. python -m pytest -q -> 180 passed, 1 skipped (pre-existing intake-eval guard, unrelated to this change); git diff --check clean.

Out of scope (untouched)

Auth/dashboard #24-#27, credentials/config, live Gitea mutations, Stripe provisioning, review checkout behavior.

Closes #28 Implements the production review-policy protocol on `GiteaClient`, plus hermetic tests. ## What landed `gitea_saas/gitea_client.py`: - `list_open_prs(repo)` — all and only open PRs (`state=open`), follows Gitea pagination (`page`/`limit`, 50/page, defensive 100-page cap), normalized shape. - `add_label(repo, issue_number, label)` / `remove_label(repo, issue_number, label)` — resolve the repo label ID, POST `{"labels": [id]}` / DELETE `/labels/{id}`, then verify the resulting label state via a re-GET. Idempotent no-op when already present/absent. Unknown label or failed verification raises `GiteaClientError` (never silently skipped). - `merge_pr(repo, pr_number) -> bool` — Gitea merge endpoint; already-merged PR short-circuits to True; after the merge request the PR is re-fetched and `merged` confirmed. Non-2xx (e.g. 409) raises. - `close_issue(repo, issue_number)` — PATCH `state: closed` and confirm the issue reports `closed`. - `get_pr()` now returns the normalized candidate shape: `{number, body, head_sha, base_ref, labels (names), repo, changed_files}` (changed_files kept for the review gate). - All non-2xx responses raise `GiteaApiError` with method/path/status/redacted body. Token and any echoed Authorization header are scrubbed from error text (defensively, even when a server echoes the request back). `tests/test_gitea_client.py` (new) — local threaded HTTP stub on 127.0.0.1; asserts request methods, paths, bodies, pagination, normalized results, failure responses, no-token-leak, and a source-derived contract test that the production client exposes every operation ReviewPolicy/the reviewer call. No live Gitea contact. ## Acceptance evidence 1. Pagination + normalization: `test_list_open_prs_follows_pagination_and_normalizes` (50-item page 1 -> page 2 -> stop; `state=open&limit=50` on every request). 2. Label mutations resolve IDs, POST/DELETE correctly, verify state; unknown label / failed verification raise: 6 dedicated tests. 3. `merge_pr` posts to `/pulls/{n}/merge` and confirms merged: 3 tests (incl. 409 raise, already-merged short-circuit). 4. `close_issue` PATCHes `{"state": "closed"}` and confirms: 2 tests. 5. Non-2xx useful error, token + Authorization header never leak (hostile echo-server test). 6. Hermetic: 22 tests over the local stub, no network beyond 127.0.0.1. 7. Contract: `test_production_client_exposes_review_policy_contract`. 8. `python -m pytest -q` -> 180 passed, 1 skipped (pre-existing intake-eval guard, unrelated to this change); `git diff --check` clean. ## Out of scope (untouched) Auth/dashboard #24-#27, credentials/config, live Gitea mutations, Stripe provisioning, review checkout behavior.
pbwraith added 1 commit 2026-08-28 20:40:17 +02:00 
Implement the review-policy protocol on the production GiteaClient:
- list_open_prs(repo): all open PRs, Gitea pagination, normalized shape
- add_label / remove_label: resolve repo label id, mutate, verify state
- merge_pr(repo, pr): merge endpoint + merged confirmation (bool)
- close_issue(repo, n): state update + closed confirmation
- get_pr() now returns the normalized candidate shape (number, body,
  head_sha, base_ref, labels as names, repo, changed_files)

Fail-closed: non-2xx responses raise GiteaApiError (token/authorization
header never leak, even when servers echo the request back); unknown
labels and failed post-condition verifications raise GiteaClientError.
Pagination has a defensive page cap.

Hermetic tests (tests/test_gitea_client.py) script a local threaded HTTP
stub and assert request methods, paths, bodies, pagination, normalized
results, failure handling, and a source-derived contract test that the
production client exposes every operation ReviewPolicy/reviewer call.
pbwraith commented 2026-08-28 21:02:42 +02:00
Author
Owner
Copy Link

Blocking: merge_pr() posts an empty object to /pulls/{n}/merge. This Gitea instance's published API schema requires MergePullRequestOption.Do, so the production request will be rejected even though the mocked test passes. The test currently locks in the invalid {} payload.

Please send a supported merge mode (at minimum {"Do": "merge"}), include head_commit_id from the reviewed PR head so a changed branch cannot be merged accidentally, and add a regression test that validates the real request shape. Re-run the full suite on the updated branch.

Blocking: `merge_pr()` posts an empty object to `/pulls/{n}/merge`. This Gitea instance's published API schema requires `MergePullRequestOption.Do`, so the production request will be rejected even though the mocked test passes. The test currently locks in the invalid `{}` payload. Please send a supported merge mode (at minimum `{"Do": "merge"}`), include `head_commit_id` from the reviewed PR head so a changed branch cannot be merged accidentally, and add a regression test that validates the real request shape. Re-run the full suite on the updated branch.
pbwraith added the
bounced
 label 2026-08-28 21:02:42 +02:00
pbwraith added 1 commit 2026-08-28 21:14:00 +02:00 
The merge request previously posted an empty object, which this Gitea
instance's published MergePullRequestOption schema rejects. Post the
supported merge mode {"Do": "merge"} plus the head_commit_id read from
the PR, so a branch that moved after review cannot be merged
accidentally. Fail closed (GiteaClientError) when the PR record carries
no head commit id. Regression tests assert the exact request shape.
pbwraith removed the
bounced
 label 2026-08-28 21:14:34 +02:00
pbwraith commented 2026-08-28 21:15:07 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 1]

Review finding (comment 10386): merge_pr() posted an empty object to /pulls/{n}/merge; tests locked in the invalid {} shape.

Fixed on the existing head branch issue-28, commit 1f2c85b (pushed):

  1. merge_pr now posts the Gitea MergePullRequestOption schema shape {"Do": "merge", "head_commit_id": <reviewed head sha>} — Do is the mandatory merge mode, and head_commit_id is read from the PR's head.sha on the pre-GET, so a branch that moved after review cannot be merged accidentally.
  2. Fail-closed: if the PR record carries no head commit id, merge_pr raises GiteaClientError instead of sending an unpinned (or empty) request.
  3. Regression tests validate the real request shape: test_merge_pr_posts_merge_and_confirms now asserts the exact payload {"Do": "merge", "head_commit_id": "sha-head"}; new test_merge_pr_sends_reviewed_head_commit_id_regression pins a custom reviewed head sha end-to-end and asserts Do is a schema-supported mode; new test_merge_pr_raises_when_pr_has_no_head_commit_id covers the fail-closed path and asserts no merge POST is attempted.

Evidence:

  • python3 -m pytest tests/test_gitea_client.py -q -> 24 passed
  • python3 -m pytest -q -> 182 passed, 1 skipped (pre-existing intake-eval guard, unrelated)
  • git diff --check -> clean
  • PR #31 re-verified after push: state=open, merged=false, head issue-28 @ 1f2c85b4fff3369ac77aef8f7aee5de8eaee13e8, 2 changed files, mergeable=true.
[worker-bounce-fix attempt 1] Review finding (comment 10386): `merge_pr()` posted an empty object to `/pulls/{n}/merge`; tests locked in the invalid `{}` shape. Fixed on the existing head branch `issue-28`, commit `1f2c85b` (pushed): 1. `merge_pr` now posts the Gitea `MergePullRequestOption` schema shape `{"Do": "merge", "head_commit_id": <reviewed head sha>}` — `Do` is the mandatory merge mode, and `head_commit_id` is read from the PR's `head.sha` on the pre-GET, so a branch that moved after review cannot be merged accidentally. 2. Fail-closed: if the PR record carries no head commit id, `merge_pr` raises `GiteaClientError` instead of sending an unpinned (or empty) request. 3. Regression tests validate the real request shape: `test_merge_pr_posts_merge_and_confirms` now asserts the exact payload `{"Do": "merge", "head_commit_id": "sha-head"}`; new `test_merge_pr_sends_reviewed_head_commit_id_regression` pins a custom reviewed head sha end-to-end and asserts `Do` is a schema-supported mode; new `test_merge_pr_raises_when_pr_has_no_head_commit_id` covers the fail-closed path and asserts no merge POST is attempted. Evidence: - `python3 -m pytest tests/test_gitea_client.py -q` -> 24 passed - `python3 -m pytest -q` -> 182 passed, 1 skipped (pre-existing intake-eval guard, unrelated) - `git diff --check` -> clean - PR #31 re-verified after push: state=open, merged=false, head `issue-28` @ `1f2c85b4fff3369ac77aef8f7aee5de8eaee13e8`, 2 changed files, mergeable=true.
pbwraith commented 2026-08-28 21:17:38 +02:00
Author
Owner
Copy Link

[governor-review] BOUNCED — head_commit_id is not the reviewed head

Re-reviewed repaired head 1f2c85b4fff3369ac77aef8f7aee5de8eaee13e8 against current base ccccfbf879bbd07512e86757abcf0c494f5b49f2 in a clean synthetic merge. Full suite: 182 passed, 1 skipped; diff check clean. The required Do: merge schema repair is correct.

One blocking TOCTOU defect remains: merge_pr() performs a fresh GET inside the merge method and uses that response's current head.sha as head_commit_id. That only guards the milliseconds between this GET and POST. It does not guard the reviewed head. If the branch moves after ReviewPolicy reviewed SHA A but before merge_pr() runs, the fresh GET returns SHA B and the client will explicitly merge unreviewed SHA B while claiming it refused the change.

Pass the expected/reviewed head SHA into the merge operation (an optional argument can preserve the existing two-argument protocol), compare the fresh PR head to it, and fail closed before POST when they differ. Send the reviewed SHA as head_commit_id. Add a regression that starts with reviewed SHA A, makes the pre-merge GET return SHA B, and proves no merge POST occurs. Keep the current missing-head and exact-schema tests.

The production policy must ultimately call this with pr["head_sha"]; coordinate that small call-site update with the retry/policy repair on PR #32 rather than weakening the client gate. Repair this same branch and PR.

[governor-review] BOUNCED — `head_commit_id` is not the reviewed head Re-reviewed repaired head `1f2c85b4fff3369ac77aef8f7aee5de8eaee13e8` against current base `ccccfbf879bbd07512e86757abcf0c494f5b49f2` in a clean synthetic merge. Full suite: 182 passed, 1 skipped; diff check clean. The required `Do: merge` schema repair is correct. One blocking TOCTOU defect remains: `merge_pr()` performs a fresh GET *inside the merge method* and uses that response's current `head.sha` as `head_commit_id`. That only guards the milliseconds between this GET and POST. It does not guard the reviewed head. If the branch moves after `ReviewPolicy` reviewed SHA A but before `merge_pr()` runs, the fresh GET returns SHA B and the client will explicitly merge unreviewed SHA B while claiming it refused the change. Pass the expected/reviewed head SHA into the merge operation (an optional argument can preserve the existing two-argument protocol), compare the fresh PR head to it, and fail closed before POST when they differ. Send the reviewed SHA as `head_commit_id`. Add a regression that starts with reviewed SHA A, makes the pre-merge GET return SHA B, and proves no merge POST occurs. Keep the current missing-head and exact-schema tests. The production policy must ultimately call this with `pr["head_sha"]`; coordinate that small call-site update with the retry/policy repair on PR #32 rather than weakening the client gate. Repair this same branch and PR.
pbwraith added the
bounced
 label 2026-08-28 21:17:39 +02:00
pbwraith added 1 commit 2026-08-28 21:34:02 +02:00
pbwraith removed the
bounced
 label 2026-08-28 21:34:54 +02:00
pbwraith commented 2026-08-28 21:34:55 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 2]

Governor review (TOCTOU) — fixed on the EXACT existing head branch issue-28 (no new branch, no duplicate PR). New commit f6b40d0 (fast-forward 1f2c85b..f6b40d0).

Finding: merge_pr() did a fresh GET inside the merge method and used that response's current head as head_commit_id — guarding only the milliseconds between that GET and POST, not the reviewed head. If the branch moved after ReviewPolicy reviewed SHA A but before merge_pr() ran, the fresh GET returns SHA B and the client would merge unreviewed SHA B while claiming it refused the change.

Fix (finding-by-finding):

  1. Reviewed head is now a first-class input — merge_pr(repo, pr, expected_head_sha=None) gains an optional argument that preserves the existing two-argument protocol (gitea_saas/gitea_client.py:344-350). The caller passes the SHA it actually reviewed.
  2. Fail closed before POST on movement — when expected_head_sha is given, the pre-merge GET's live head is compared to it and any mismatch raises GiteaClientError BEFORE the merge endpoint is touched (:373-378). No POST means unreviewed SHA B can never be merged.
  3. Reviewed SHA is what gets pinned — head_commit_id is the caller-supplied reviewed SHA, not the live GET value (:380-382).
  4. Regression tests — test_merge_pr_refuses_moved_head_before_post starts with reviewed SHA A, makes the pre-merge GET return SHA B, and proves no merge POST occurs (plus no payload leak); test_merge_pr_pins_expected_head_in_payload proves the explicit reviewed SHA flows into head_commit_id. Existing missing-head and exact-schema tests (test_merge_pr_raises_when_pr_has_no_head_commit_id, test_merge_pr_posts_merge_and_confirms, test_merge_pr_sends_reviewed_head_commit_id_regression) are kept unchanged and still pass.
  5. Policy call-site coordination — per the governor's note, the call-site update (review_policy passing pr["head_sha"]) is deliberately NOT added to this client PR; it belongs with the retry/policy repair tracked on PR #32 (issue #29), which already carries the transactional recovery work. The client gate is not weakened: untaken callers keep working (two-arg form still pins the live head), and the policy path will call the pinned form.

Evidence:

  • python3 -m pytest tests/test_gitea_client.py -q: 26 passed (24 prior + 2 new)
  • python3 -m pytest -q: 184 passed, 1 skipped (pre-existing intake-eval guard)
  • git diff --check: clean
  • PR #31 re-verified after push: state=open, merged=false, head issue-28 @ f6b40d0, 2 changed files.

PR left open for manual review; not merged by worker.

[worker-bounce-fix attempt 2] Governor review (TOCTOU) — fixed on the EXACT existing head branch `issue-28` (no new branch, no duplicate PR). New commit `f6b40d0` (fast-forward `1f2c85b..f6b40d0`). **Finding:** `merge_pr()` did a fresh GET inside the merge method and used that response's current head as `head_commit_id` — guarding only the milliseconds between that GET and POST, not the reviewed head. If the branch moved after `ReviewPolicy` reviewed SHA A but before `merge_pr()` ran, the fresh GET returns SHA B and the client would merge unreviewed SHA B while claiming it refused the change. **Fix (finding-by-finding):** 1. *Reviewed head is now a first-class input* — `merge_pr(repo, pr, expected_head_sha=None)` gains an optional argument that preserves the existing two-argument protocol (`gitea_saas/gitea_client.py:344-350`). The caller passes the SHA it actually reviewed. 2. *Fail closed before POST on movement* — when `expected_head_sha` is given, the pre-merge GET's live head is compared to it and any mismatch raises `GiteaClientError` BEFORE the merge endpoint is touched (`:373-378`). No POST means unreviewed SHA B can never be merged. 3. *Reviewed SHA is what gets pinned* — `head_commit_id` is the caller-supplied reviewed SHA, not the live GET value (`:380-382`). 4. *Regression tests* — `test_merge_pr_refuses_moved_head_before_post` starts with reviewed SHA A, makes the pre-merge GET return SHA B, and proves no merge POST occurs (plus no payload leak); `test_merge_pr_pins_expected_head_in_payload` proves the explicit reviewed SHA flows into `head_commit_id`. Existing missing-head and exact-schema tests (`test_merge_pr_raises_when_pr_has_no_head_commit_id`, `test_merge_pr_posts_merge_and_confirms`, `test_merge_pr_sends_reviewed_head_commit_id_regression`) are kept unchanged and still pass. 5. *Policy call-site coordination* — per the governor's note, the call-site update (`review_policy` passing `pr["head_sha"]`) is deliberately NOT added to this client PR; it belongs with the retry/policy repair tracked on PR #32 (issue #29), which already carries the transactional recovery work. The client gate is not weakened: untaken callers keep working (two-arg form still pins the live head), and the policy path will call the pinned form. **Evidence:** - `python3 -m pytest tests/test_gitea_client.py -q`: **26 passed** (24 prior + 2 new) - `python3 -m pytest -q`: **184 passed, 1 skipped** (pre-existing intake-eval guard) - `git diff --check`: clean - PR #31 re-verified after push: state=open, merged=false, head `issue-28` @ `f6b40d0`, 2 changed files. PR left open for manual review; not merged by worker.
pbwraith merged commit fc55c41dd0 into main 2026-08-29 09:51:20 +02:00
pbwraith commented 2026-08-29 09:51:24 +02:00
Author
Owner
Copy Link

[governor-review] MERGED

Reviewed exact head f6b40d010820e6ff48f3f4d9411383642e1b1656 against base ccccfbf879bbd07512e86757abcf0c494f5b49f2 in a fresh synthetic merge. git diff --check was clean and the full suite passed: 184 passed, 1 skipped.

The production merge request now uses Gitea's supported Do: merge shape, can pin the caller's reviewed head, rejects a moved head before POST, redacts API errors, verifies label/close mutations, and has hermetic request-shape coverage. PR #32 remains separately bounced until its policy call site passes this reviewed SHA and its recovery path positively distinguishes a merged PR.

Merged with head_commit_id pinned to the reviewed SHA; linked issue #28 closed.

[governor-review] MERGED Reviewed exact head `f6b40d010820e6ff48f3f4d9411383642e1b1656` against base `ccccfbf879bbd07512e86757abcf0c494f5b49f2` in a fresh synthetic merge. `git diff --check` was clean and the full suite passed: 184 passed, 1 skipped. The production merge request now uses Gitea's supported `Do: merge` shape, can pin the caller's reviewed head, rejects a moved head before POST, redacts API errors, verifies label/close mutations, and has hermetic request-shape coverage. PR #32 remains separately bounced until its policy call site passes this reviewed SHA and its recovery path positively distinguishes a merged PR. Merged with `head_commit_id` pinned to the reviewed SHA; linked issue #28 closed.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked

   
bounced

   
eval-task

   
in-progress

   
in-review

   
needs-guidance

   
p1

   
p2

   
p3

   
tracking

   
unfunded
No Label
blocked
bounced
eval-task
in-progress
in-review
needs-guidance
p1
p2
p3
tracking
unfunded
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: pbwraith/gitea-saas-core#31
No description provided.
Delete Branch "issue-28"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?