Review gate: replaces the arbitrary repo_dir test target with a
review_workspace seam that builds a clean temporary git worktree from
the configured clone, fetches the exact base/head SHAs, and constructs
a real synthetic merge of the reviewed head onto the reviewed base.
Verdicts pin repo, PR, head SHA, and base SHA; tests + git diff --check
run inside the workspace with an explicit base..HEAD range; a merge
conflict yields a deterministic BOUNCE with conflicted-file evidence and
the suite is never run against a fallback checkout; no workspace
configured fails closed to NEEDS_GUIDANCE. Worktrees/refs are cleaned up
on success, conflict, timeout, and exceptions; the configured clone and
worker issue worktrees are never mutated.
Policy: before applying any verdict the PR is re-read and the verdict is
refused (outcome=stale/PAIR_MOVED, zero side effects) if the head OR
base moved after evaluation — the new pair stays eligible for a fresh
review. Client: normalized PR shape gains base_sha (REVIEW_CLONE_DIR is
the new config knob). Hermetic tests build local temp repos proving
clean merge, conflict, moved head/base, no shared-checkout mutation,
correct command cwd/range, and cleanup on failure (no network).
Governor review 10539 findings, finding by finding (issue-29 branch, PR #32 retained):
1. Recovery must never attribute a DIFFERENT head's merge to a reviewed-head plan.
recover_incomplete() adopted any live record with merged is True for the PR
number without comparing heads: if plan A was persisted and then head B was
merged externally, the retry row for A would debit/label/close/comment as
though reviewed head A had been delivered. Crash-window adoption now
requires BOTH positive merged proof AND live head_sha == the persisted
planned/reviewed head_sha. A merged DIFFERENT head fails closed with a
structured HEAD_MISMATCH result (persisted attempt + result entry), and
performs NO debit, NO label removal, NO issue close, NO success comment;
the row is left unadopted. The same-head crash-window success case is
preserved.
2. A candidate with no reviewed head_sha must never reach the merge client.
Passing expected_head_sha=None to merge_pr would arm the production
client's backward-compatible live-head fallback and could merge a head the
policy never reviewed. _apply_policy() now rejects a missing/empty
head_sha BEFORE the plan write (review_state.head_sha is NOT NULL, so the
DB would reject it anyway — now with the correct semantic failure) and
_safe_merge() independently refuses to call the client without a pinned
reviewed head. Zero merge attempts and zero downstream side effects.
New regressions (tests/test_review_policy.py):
- test_recovery_merged_different_head_fails_closed — plan at sha-A, live
merged at sha-B: HEAD_MISMATCH, zero side effects, merged_step unmarked.
- test_missing_reviewed_head_never_reaches_merge_client — candidate with no
head_sha: structured failure, no merge_pr call, no debit/label/close/comment.
Preserved: reviewed-SHA merge pin, production normalized merged/state shape,
closed-unmerged negative gate (state==closed never counts), durable recovery
independent of the open-PR scan, checkpoint/idempotency coverage, and the
same-head crash-window adoption.
Evidence: python3 -m pytest -q -> 203 passed, 1 skipped (was 201 passed, 1
skipped; +2 new tests). git diff --check clean. Files: gitea_saas/review_policy.py,
tests/test_review_policy.py.
Governor review #2 findings on PR #32:
1. _safe_merge now passes the REVIEWED head sha to
gitea.merge_pr(..., expected_head_sha=...) — the production client
(issue #28) fails closed before any POST when the branch moved after
review, so an unreviewed SHA can never be merged. Integration
regression: SHA A reviewed / live SHA B -> no merge POST, no debit,
no close; the moved head is re-reviewed and merged next poll.
2. Production client shape coordinated: GiteaClient._normalize_pr now
carries the raw 'merged' boolean and 'state' in every normalized PR
(list_open_prs and get_pr), so recover_incomplete can positively
identify a merged PR. Tests exercise the production-shaped fake
(merged/state fields, open-only list_open_prs, live-record mutation
on merge) instead of a richer test-only dict. Crash-window adoption
regression: merged_pr checkpoint missed, live record reports
merged=True -> recovery finishes debit/label/close/comment without a
second merge request.
3. state == 'closed' is no longer treated as merged anywhere: _safe_merge
and recover_incomplete adopt a merge only on 'merged is True'.
Negative regression: a closed-but-unmerged PR with a pending outcome
plan produces no debit, no label removal, no issue close, no success
comment.
Evidence: 201 passed, 1 skipped (full suite); git diff --check clean.
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.
A head SHA is now recorded as completed (reviews table) only after its
required policy action fully succeeded; any failure — verdict generation
or a label/comment/merge/close/debit boundary — is persisted in the new
review_state table (attempts, last_error, merged_step/closed_step
checkpoints) and reported as a structured outcome=failed result, leaving
the same unchanged head eligible for retry on the next poll.
Retries resume from checkpoints so a partial side effect can never
double-merge a PR or double-close an issue; the delivery debit keeps its
billing-core idempotency key (<repo>#<pr>@<head_sha>). Legacy reviews
rows are preserved untouched (completed history); no migration needed.
Closes#29
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.
The guard asserted gitea_saas/server.py was absent from the tree. That held
only until #2 (which owns the app factory) merged; afterwards it fired against
#2's own file rather than anything #9 shipped, and was the single failure when
issue-9 was merged onto main.
issue-9 adds exactly two files (gitea_saas/dashboard.py, tests/test_dashboard.py)
and never contained server.py, so the invariant the docstring describes was
always satisfied. It is now checked directly, via AST, so mount_dashboard's
docstring example (app = Flask(__name__)) is not mistaken for a real call.
Review finding (2026-08-26): the dashboard was the only auth path in the
service NOT using hmac.compare_digest — plain str != short-circuits on the
first differing byte (prefix timing leak on the shared secret) — and the
header name was a hardcoded literal instead of a named constant.
- import hmac; compare via hmac.compare_digest (503 fail-closed branch
kept exactly as reviewed)
- API_KEY_HEADER module constant is the single source of truth; when #24
consolidates auth paths this line becomes an import of the shared
config constant (config.py lands with #2/#6, both unmerged)
- new test: a wrong key of the SAME length as the configured key must be
rejected (the length-differing fixture cannot distinguish != from
compare_digest)
Bounce #2 (2026-08-25 22:38): gitea_saas/gitea_client.py is not in #8's
Files-allowed set. It was reassigned to issue #4 (sole owner) by !23 which
closed#21. Removed from this branch entirely; the import resolves from
main once !17 lands (reviewer: "only who ships the module changes").
The canonical client (issue-4 branch) is missing list_issue_comments —
the exact 8-line delta the bounce predicted. Per bounce instruction #3,
that is NOT forked here; the missing method is being requested on issue #4.
test_real_client_list_issue_comments_builds_correct_request now skips with
an explicit reason until the canonical client grows the method, so the gate
stays green and the guard re-activates automatically once !17 lands.
Review findings (manual review of !22):
1. Main moved: #16 renamed create_client/get_client records to flat
id-keyed shape. Tests read the old nested ['client']/['client_id']
shape (10 collection errors). Fixed to the flat ['id'] record;
parameter names stay unchanged per review.
2. server.py three-way conflict (#2/#6/#9): issue #9 must NOT define a
second Flask app. gitea_saas/server.py is dropped from this branch;
dashboard.py now exposes mount_dashboard(app, core, db_path, api_key)
which #2's app factory wires in. Endpoint tests build a throwaway app
the same way (X-API-Key discipline, fail-closed 503, 401) and add a
mount-contract test asserting no server.py ships with this issue.
Rebased onto current main (bcf03bd). Full gate: 58 passed, diff clean.
Addendum finding (bounce comment 9000): _authorized compared the shared
secret with ==, leaking a prefix-match timing signal. Use
hmac.compare_digest and read the header name from the shared
API_KEY_HEADER constant (defensive import; gitea_saas/config.py lands
with sibling auth PRs, fallback X-API-Key keeps this module importable
until then). Route comment updated to state the real discipline.
46 tests pass.
main (#16) flattened get_client records and renamed create_client's
returned identifier to 'id'. Read create_client(...)['id'] in the
real-core composition test, and make _default_repo_name accept the flat
record (top-level name / id) as well as the historical nested shape.
46 tests pass on top of origin/main.
BillingCore.create_client now returns the flat record keyed as 'id' (main's
PR #16/#19 rename). The Stripe test fixtures were reading the old
'client_id' return key, failing 5 tests on current main. Only the return
value changed; the 'client_id' parameter to member_payload and the
client_id metadata key are untouched.
GiteaClient (shared seam): create_org_repo/repo_exists/create_issue plus
the review-gate surface (get_issue/get_pr/get_pr_diff/post_comment).
IntakeFactory: provision_client (idempotent), submit_issue (funded ships,
unfunded/locked queues), unlock (flush with p1), lock, client_unfunded.
Factory tables (client_repos, pending_issues, factory_locks) are created
idempotently against the billing SQLite file. register_factory_routes
mounts the /api/factory/* endpoints on the issue #2 Flask app.
Gates live in the service only: no dispatcher/lane-script coupling.
Replace the == string comparison at delivery.py:127 with
hmac.compare_digest so the shared secret never leaks through response
timing (repo convention: stripe_client.py:88, server.py:171). The
isinstance/empty guards run first so compare_digest's TypeError on
non-str input is never reached — unset config, None, and non-str keys
stay fail-closed False. Five direct authorized() tests added (16
delivery tests / 42 full suite green).
create_client now returns 'id' (was 'client_id'), get_client returns a flat
record with balance as a field (was nested under 'client'), and list_clients
is implemented returning {'clients': [...]} with the same flat shape. A
cross-method shape-consistency test (create/get/list key-set agreement)
plus a wire-shape test mirroring the server's _client_out allow-list guard
against future drift. server.py/test_server.py untouched (issue #2/!13).
build_stripe_client() no longer degrades to the mock on absent config.
Mock is selected only on explicit opt-in (mock=True or SAAS_STRIPE_MODE=mock);
with mode unset/live, missing STRIPE_SECRET_KEY / STRIPE_WEBHOOK_SECRET raise
StripeConfigError naming the variable (never its value). MockStripeClient's
webhook_signature is now required (no published default) and verified with
hmac.compare_digest. Regression test asserts the forged-webhook scenario
leaves the balance unchanged.
