gitea-saas-core#30: isolated synthetic-merge review workspace #37

pbwraith · 2026-08-29T22:22:28+02:00

pbwraith commented

2026-08-29 22:22:28 +02:00

Closes #30 — the review gate now evaluates the exact synthetic merge result in an isolated workspace.

What changed

New: gitea_saas/review_workspace.py — ReviewWorkspace(clone_dir, runner) builds a clean, isolated, temporary Git worktree per review:

fetches the named base SHA + PR head SHA from the configured clone's origin and creates the worktree detached at the base;
runs a real git merge --no-ff of the exact head onto the exact base inside that worktree (the synthetic merge result; HEAD = the merge commit);
a genuine merge conflict returns deterministic evidence (conflicted_files, status summary, merge_ok=False) and the test suite is NEVER run against any fallback checkout;
cleanup (worktree remove --force + rmtree) runs on success, conflict, timeout, and raised exceptions; no branch refs are ever created (detached), so no temporary refs survive.

gitea_saas/reviewer.py — review_pr(..., workspace=...):

every verdict pins repo, PR number, head SHA and current base SHA (acceptance 1);
tests, build, and git diff --check <base> HEAD (explicit base/result range, acceptance 5) run with cwd = the isolated workspace (acceptance 2/3);
no workspace configured (or no resolvable SHAs) fails closed to NEEDS_GUIDANCE — a green verdict can no longer describe the wrong checkout;
the review-gate still never calls merge (the AST guard test still passes; the evidence field was deliberately named workspace).

gitea_saas/review_policy.py — before applying any verdict the PR is re-read and the verdict is refused when EITHER the head or the base moved after evaluation (outcome="stale", PAIR_MOVED, zero side effects — acceptance 6); the new pair is eligible for a fresh review. The policy builds the workspace from the new REVIEW_CLONE_DIR config knob.

gitea_saas/gitea_client.py — the normalized PR shape now carries base_sha (alongside head_sha).

gitea_saas/config.py — REVIEW_CLONE_DIR (README: the READ-ONLY repository clone used to build the isolated review workspace).

Hermetic tests (acceptance 8 — local temp git repos, zero network)

tests/test_review_workspace.py (new): clean synthetic merge (both sides' files present, HEAD is a real merge commit, evidence pinned); deterministic merge conflict; the configured clone AND a worker-style issue worktree are never mutated (dirty state survives); cleanup on raised exception, conflict exit, unfetchable SHA, and non-hex inputs; all git commands run in the right cwd (merge inside the workspace, fetch/worktree ops against the clone); explicit-range diff --check from the workspace dir.
tests/test_reviewer.py: FakeWorkspace seam; merge conflict -> BOUNCE with evidence and NO test run; no workspace -> NEEDS_GUIDANCE; comment pins head+base; hygiene command is the explicit git diff --check <base> HEAD; verdict/comment shape untouched.
tests/test_review_policy.py: evaluated head move and evaluated base move each refuse the verdict with zero side effects and the new pair gets a fresh chance; workspace passed through when a clone is configured; all fakes mirror the production shape (head_sha + base_sha).

Gates

python -m pytest -q — 218 passed, 1 skipped (pre-existing skip in test_intake_eval, unrelated).
git diff --check — clean.

Out of scope honored: no auth/dashboard PRs, no credential/hosted-service changes, no live Gitea mutation tests, no Stripe changes, no evidence/LLM prompt redesign.

Closes #30 — the review gate now evaluates the exact synthetic merge result in an isolated workspace. ## What changed **New: `gitea_saas/review_workspace.py`** — `ReviewWorkspace(clone_dir, runner)` builds a clean, isolated, temporary Git worktree per review: - fetches the named base SHA + PR head SHA from the configured clone's `origin` and creates the worktree detached at the base; - runs a real `git merge --no-ff` of the exact head onto the exact base inside that worktree (the synthetic merge result; HEAD = the merge commit); - a genuine merge conflict returns deterministic evidence (`conflicted_files`, status summary, `merge_ok=False`) and the test suite is NEVER run against any fallback checkout; - cleanup (worktree remove --force + rmtree) runs on success, conflict, timeout, and raised exceptions; no branch refs are ever created (detached), so no temporary refs survive. **`gitea_saas/reviewer.py`** — `review_pr(..., workspace=...)`: - every verdict pins repo, PR number, head SHA and current base SHA (acceptance 1); - tests, build, and `git diff --check <base> HEAD` (explicit base/result range, acceptance 5) run with cwd = the isolated workspace (acceptance 2/3); - no workspace configured (or no resolvable SHAs) fails closed to NEEDS_GUIDANCE — a green verdict can no longer describe the wrong checkout; - the review-gate still never calls merge (the AST guard test still passes; the evidence field was deliberately named `workspace`). **`gitea_saas/review_policy.py`** — before applying any verdict the PR is re-read and the verdict is refused when EITHER the head or the base moved after evaluation (`outcome="stale"`, `PAIR_MOVED`, zero side effects — acceptance 6); the new pair is eligible for a fresh review. The policy builds the workspace from the new `REVIEW_CLONE_DIR` config knob. **`gitea_saas/gitea_client.py`** — the normalized PR shape now carries `base_sha` (alongside `head_sha`). **`gitea_saas/config.py`** — `REVIEW_CLONE_DIR` (README: the READ-ONLY repository clone used to build the isolated review workspace). ## Hermetic tests (acceptance 8 — local temp git repos, zero network) - `tests/test_review_workspace.py` (new): clean synthetic merge (both sides' files present, HEAD is a real merge commit, evidence pinned); deterministic merge conflict; the configured clone AND a worker-style issue worktree are never mutated (dirty state survives); cleanup on raised exception, conflict exit, unfetchable SHA, and non-hex inputs; all git commands run in the right cwd (merge inside the workspace, fetch/worktree ops against the clone); explicit-range diff --check from the workspace dir. - `tests/test_reviewer.py`: FakeWorkspace seam; merge conflict -> BOUNCE with evidence and NO test run; no workspace -> NEEDS_GUIDANCE; comment pins head+base; hygiene command is the explicit `git diff --check <base> HEAD`; verdict/comment shape untouched. - `tests/test_review_policy.py`: evaluated head move and evaluated base move each refuse the verdict with zero side effects and the new pair gets a fresh chance; workspace passed through when a clone is configured; all fakes mirror the production shape (head_sha + base_sha). ## Gates - `python -m pytest -q` — 218 passed, 1 skipped (pre-existing skip in test_intake_eval, unrelated). - `git diff --check` — clean. Out of scope honored: no auth/dashboard PRs, no credential/hosted-service changes, no live Gitea mutation tests, no Stripe changes, no evidence/LLM prompt redesign.

pbwraith added 1 commit 2026-08-29 22:22:30 +02:00

gitea-saas-core#30: isolated synthetic-merge review workspace d8faabf2e5

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

pbwraith added the

in-review

label 2026-08-29 22:22:47 +02:00

pbwraith commented

2026-08-29 22:22:49 +02:00

[worker-implemented] gitea-saas-core#30 — commit d8faabf on branch issue-30 (base main), PR #37.

Evidence:

python -m pytest -q — 218 passed, 1 skipped (pre-existing test_intake_eval skip, unrelated).
git diff --check — clean.
New hermetic workspace suite (local temp git repos, no network): clean merge, deterministic merge conflict, no shared-checkout/worker-worktree mutation, cleanup on exception/conflict/unfetchable-SHA/non-hex inputs, command cwd + explicit base..HEAD range assertions.
Policy suite: evaluated head OR base movement refuses the verdict (outcome=stale, PAIR_MOVED) with zero side effects; the new pair is fresh-eligible.

Acceptance cross-check: 1 (verdict pins repo/pr/head/base) ✓ · 2 (isolated temp worktree from the configured clone; shared main/worker worktrees untouched) ✓ · 3 (fetches named base+head, synthetic merge of exact head onto current base) ✓ · 4 (conflict -> deterministic BOUNCE with evidence, no fallback test run) ✓ · 5 (tests + git diff --check against the merge result with explicit base/result range) ✓ · 6 (policy re-reads the PR; refuses when head or base moved; new pair reviewed fresh) ✓ · 7 (cleanup on success, test failure, conflict, timeout, exceptions) ✓ · 8 (hermetic local-repo tests proving all of the above) ✓ · 9 (pytest + git diff --check green) ✓

[worker-implemented] gitea-saas-core#30 — commit `d8faabf` on branch `issue-30` (base `main`), PR #37. Evidence: - `python -m pytest -q` — 218 passed, 1 skipped (pre-existing test_intake_eval skip, unrelated). - `git diff --check` — clean. - New hermetic workspace suite (local temp git repos, no network): clean merge, deterministic merge conflict, no shared-checkout/worker-worktree mutation, cleanup on exception/conflict/unfetchable-SHA/non-hex inputs, command cwd + explicit base..HEAD range assertions. - Policy suite: evaluated head OR base movement refuses the verdict (`outcome=stale`, `PAIR_MOVED`) with zero side effects; the new pair is fresh-eligible. Acceptance cross-check: 1 (verdict pins repo/pr/head/base) ✓ · 2 (isolated temp worktree from the configured clone; shared main/worker worktrees untouched) ✓ · 3 (fetches named base+head, synthetic merge of exact head onto current base) ✓ · 4 (conflict -> deterministic BOUNCE with evidence, no fallback test run) ✓ · 5 (tests + git diff --check against the merge result with explicit base/result range) ✓ · 6 (policy re-reads the PR; refuses when head or base moved; new pair reviewed fresh) ✓ · 7 (cleanup on success, test failure, conflict, timeout, exceptions) ✓ · 8 (hermetic local-repo tests proving all of the above) ✓ · 9 (pytest + git diff --check green) ✓

pbwraith commented

2026-08-30 00:58:48 +02:00

[review-gate] VERDICT: BOUNCE

Reviewed exact pair: head d8faabf2e53fac2657b4f997d10247cec141d1f8 onto base b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 as a clean synthetic merge.

Evidence:

python -m pytest -q: 233 passed, 1 skipped
git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD: clean

Blocking finding:

ReviewWorkspace interpolates tmpdir directly into commands executed by Runner(shell=True):
- git worktree add --detach {tmpdir} {base_sha}
- git worktree remove --force {tmpdir}
A valid OS temp path containing spaces or shell metacharacters is therefore split/interpreted by the shell. Construction fails for paths with spaces; more importantly, cleanup can remove the directory with shutil.rmtree while failing to remove Git's worktree administration record. That violates acceptance criterion 7: clean up all temporary paths/worktree state on success, conflict, timeout, and exceptions. This is also relevant to Windows deployments, where space-containing paths are routine.

Required repair on the same branch:

Shell-quote every filesystem path interpolated into a runner command (for example with shlex.quote(str(path))), including add and remove commands. Keep the already-strict SHA validation.
Make the mkdtemp prefix filesystem-safe when repo contains separators or punctuation.
Add a hermetic real-Git test whose temporary/worktree root contains spaces (and preferably a shell-sensitive character) and prove both successful construction and complete cleanup, including absence from git worktree list --porcelain after exit.
Re-run the full suite and git diff --check against the current base.

The synthetic-merge design and pair-movement guard otherwise look sound. Please repair this PR on its existing head branch; do not open a replacement PR.

[review-gate] VERDICT: **BOUNCE** Reviewed exact pair: head `d8faabf2e53fac2657b4f997d10247cec141d1f8` onto base `b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02` as a clean synthetic merge. Evidence: - `python -m pytest -q`: **233 passed, 1 skipped** - `git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD`: clean Blocking finding: - `ReviewWorkspace` interpolates `tmpdir` directly into commands executed by `Runner(shell=True)`: - `git worktree add --detach {tmpdir} {base_sha}` - `git worktree remove --force {tmpdir}` A valid OS temp path containing spaces or shell metacharacters is therefore split/interpreted by the shell. Construction fails for paths with spaces; more importantly, cleanup can remove the directory with `shutil.rmtree` while failing to remove Git's worktree administration record. That violates acceptance criterion 7: clean up all temporary paths/worktree state on success, conflict, timeout, and exceptions. This is also relevant to Windows deployments, where space-containing paths are routine. Required repair on the same branch: 1. Shell-quote every filesystem path interpolated into a runner command (for example with `shlex.quote(str(path))`), including add and remove commands. Keep the already-strict SHA validation. 2. Make the `mkdtemp` prefix filesystem-safe when `repo` contains separators or punctuation. 3. Add a hermetic real-Git test whose temporary/worktree root contains spaces (and preferably a shell-sensitive character) and prove both successful construction and complete cleanup, including absence from `git worktree list --porcelain` after exit. 4. Re-run the full suite and `git diff --check` against the current base. The synthetic-merge design and pair-movement guard otherwise look sound. Please repair this PR on its existing head branch; do not open a replacement PR.

pbwraith added the

bounced

label 2026-08-30 00:58:49 +02:00

pbwraith added 1 commit 2026-08-30 01:12:00 +02:00

fix(#30 ): shell-quote workspace paths + sanitize temp prefix (bounce-fix) 069a74a95c

The review gate builds the synthetic merge in a temporary worktree whose
path was interpolated raw into Runner(shell=True) commands: paths with
spaces or shell metacharacters were split by the shell, so git's worktree
admin entry could survive cleanup (criterion 7 violation; CWD was also
misleading on failure).

- shlex.quote the worktree path in 'git worktree add --detach' and
  'git worktree remove --force'; SHA validation is unchanged.
- sanitize the mkdtemp prefix token ('owner/name' -> 'owner-name'), so a
  repo name with separators/punctuation cannot make mkdtemp fail.
- hermetic real-git tests: temp root containing a space and '$'
  constructs and cleans up completely (dir gone + absent from 'git
  worktree list --porcelain'); repo token with separators builds a
  single-component prefix and cleans up.

pbwraith removed the

bounced

label 2026-08-30 01:13:17 +02:00

pbwraith commented

2026-08-30 01:13:19 +02:00

[worker-bounce-fix attempt 1] gitea-saas-core#37 — head 069a74a9 (branch issue-30) repairs the review-gate BOUNCE verdict of 2026-08-29.

Finding-by-finding:

Shell-quoted filesystem paths — git worktree add --detach {tmpdir} {base_sha} and git worktree remove --force {tmpdir} now interpolate shlex.quote(str(tmpdir)) for both add and remove (commands go through the same Runner(shell=True) seam). The already-strict 40-hex SHA validation is unchanged; only path tokens are quoted.
Filesystem-safe mkdtemp prefix — the repo token embedded in the prefix is collapsed through _fs_safe_token ([^A-Za-z0-9_-]+ -> -, trimmed), so owner/name yields review-ws-owner-name-<n>-*; separators/punctuation can no longer turn the prefix into a nested path that makes tempfile.mkdtemp fail.
New hermetic real-Git tests (local bare origin, zero network):
- test_weird_temp_root_with_spaces_and_shell_char_success_and_cleanup — temp root review root $weird (space + shell-sensitive $): construction succeeds (merge_ok=True), and after exit the directory is gone AND the clone's git worktree list --porcelain contains no review-ws entry (no dangling worktree admin record).
- test_repo_token_with_separators_is_filesystem_safe — repo pbwraith/gitea-saas-core builds a single-component temp prefix and cleans up completely.
Gate evidence on the exact pair — head 069a74a95ce6989fe6a4eb35555d79d7a5c2a2ff onto base b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 as a clean synthetic merge:
- python -m pytest -q — 235 passed, 1 skipped (pre-existing intake skip).
- git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD — clean.
- tests/test_review_workspace.py — 11 passed inside the merged tree.

The synthetic-merge design and pair-movement guard are unchanged. PR #37 re-verified after push: state=open, merged=false, head issue-30 @ 069a74a, 9 changed files. Left open for manual review.

[worker-bounce-fix attempt 1] gitea-saas-core#37 — head `069a74a9` (branch `issue-30`) repairs the review-gate BOUNCE verdict of 2026-08-29. Finding-by-finding: 1. **Shell-quoted filesystem paths** — `git worktree add --detach {tmpdir} {base_sha}` and `git worktree remove --force {tmpdir}` now interpolate `shlex.quote(str(tmpdir))` for both add and remove (commands go through the same `Runner(shell=True)` seam). The already-strict 40-hex SHA validation is unchanged; only path tokens are quoted. 2. **Filesystem-safe mkdtemp prefix** — the `repo` token embedded in the prefix is collapsed through `_fs_safe_token` (`[^A-Za-z0-9_-]+` -> `-`, trimmed), so `owner/name` yields `review-ws-owner-name-<n>-*`; separators/punctuation can no longer turn the prefix into a nested path that makes `tempfile.mkdtemp` fail. 3. **New hermetic real-Git tests** (local bare origin, zero network): - `test_weird_temp_root_with_spaces_and_shell_char_success_and_cleanup` — temp root `review root $weird` (space + shell-sensitive `$`): construction succeeds (`merge_ok=True`), and after exit the directory is gone AND the clone's `git worktree list --porcelain` contains no `review-ws` entry (no dangling worktree admin record). - `test_repo_token_with_separators_is_filesystem_safe` — repo `pbwraith/gitea-saas-core` builds a single-component temp prefix and cleans up completely. 4. **Gate evidence on the exact pair** — head `069a74a95ce6989fe6a4eb35555d79d7a5c2a2ff` onto base `b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02` as a clean synthetic merge: - `python -m pytest -q` — **235 passed, 1 skipped** (pre-existing intake skip). - `git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD` — clean. - `tests/test_review_workspace.py` — 11 passed inside the merged tree. The synthetic-merge design and pair-movement guard are unchanged. PR #37 re-verified after push: `state=open`, `merged=false`, head `issue-30` @ `069a74a`, 9 changed files. Left open for manual review.

pbwraith merged commit 71cc2634ce into main

2026-08-30 08:08:41 +02:00

pbwraith referenced this issue from a commit

2026-08-30 08:08:43 +02:00

Merge pull request 'gitea-saas-core#30: isolated synthetic-merge review workspace' (#37) from issue-30 into main

pbwraith removed the

in-review

label 2026-08-30 08:08:44 +02:00

pbwraith commented

2026-08-30 08:08:46 +02:00

[review-gate] VERDICT: MERGE

Reviewed exact synthetic merge: head 069a74a95ce6989fe6a4eb35555d79d7a5c2a2ff onto base b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02.

Final evidence:

python3 -m pytest -q: 235 passed, 1 skipped
git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD: clean
New real-Git test constructs and removes a synthetic-merge worktree below a temp root containing both spaces and $.
After context exit, the worktree directory is absent and git worktree list --porcelain contains no stale review entry.
Repository tokens containing separators are collapsed into one filesystem-safe temp-prefix component.
Add/remove filesystem paths are shell-quoted; strict 40-hex SHA validation and pair-movement guards remain intact.

All issue #30 acceptance criteria are satisfied on the current merge result. Merging and closing the linked issue.

[review-gate] VERDICT: **MERGE** Reviewed exact synthetic merge: head `069a74a95ce6989fe6a4eb35555d79d7a5c2a2ff` onto base `b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02`. Final evidence: - `python3 -m pytest -q`: **235 passed, 1 skipped** - `git diff --check b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02 HEAD`: clean - New real-Git test constructs and removes a synthetic-merge worktree below a temp root containing both spaces and `$`. - After context exit, the worktree directory is absent and `git worktree list --porcelain` contains no stale review entry. - Repository tokens containing separators are collapsed into one filesystem-safe temp-prefix component. - Add/remove filesystem paths are shell-quoted; strict 40-hex SHA validation and pair-movement guards remain intact. All issue #30 acceptance criteria are satisfied on the current merge result. Merging and closing the linked issue.

Sign in to join this conversation.

No reviewers