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

Review gate: automated PR reviewer (acceptance parse, tests, verdicts) (#5) #12

Merged
pbwraith merged 2 commits from issue-5 into main 2026-08-25 18:14:18 +02:00
Conversation 6 Commits 2 Files Changed 2 +696
pbwraith commented 2026-08-25 11:31:25 +02:00
Owner
Copy Link

Closes #5

Review gate: the automated PR reviewer. No dependencies — built directly on main (docs-only baseline).

Files (only the allowed set):

  • gitea_saas/gitea_client.py — Gitea API seam (urllib, token from env, never on a command line): get_issue, get_pr (head_sha + changed_files), get_pr_diff, post_comment. Real HTTP only when a token exists; tests use an in-memory fake.
  • gitea_saas/reviewer.py — Reviewer(repo_client, runner) with review_pr(repo, pr, linked_issue, repo_dir) -> Verdict and post_verdict(...) (posts [review-gate] VERDICT: ... comment with the acceptance checklist + findings).
  • tests/test_reviewer.py — 16 hermetic tests.

Pipeline per review: fetch PR head sha / files / diff -> parse acceptance checklist (- [ ]/- [x], or ## Acceptance section) -> parse Files allowed scope -> run python -m pytest -q (REVIEW_TEST_TIMEOUT_S, default 300) -> optional build cmd (REVIEW_BUILD_CMD) -> git diff --check -> findings pass (deterministic secret/TODO scan by default; configurable LLM pass validated so unquoted findings are dropped) -> verdict.

Verdict rules: tests non-zero -> BOUNCE (unless allow_test_failure in the issue body); timeout -> BOUNCE with tests.exit = "timed_out"; scope violation -> BOUNCE; blocking finding -> BOUNCE; empty acceptance -> NEEDS_GUIDANCE (never silent-approve); else APPROVE.

Fail-closed guarantees:

  • Findings must quote the diff (evidence_quote), redacting leaked secret values; the no-LLM default is fully deterministic.
  • No merge call anywhere in reviewer.py (AST-checked by a test); the posted comment only disclaims merging — "No merge performed by the review gate (policy/merge is issue #7)".
  • Test grounding: approve-check, bounce-on-tests, bounce-on-scope, needs-guidance-on-no-acceptance, allow_test_failure override, timeout -> BOUNCE (Runner timeout path tested hermetically via injected command runner), secret/TODO findings quoting the diff, build cmd + build-failure bounce, diff-hygiene blocking finding, verdict shape, no-merge AST scan.

Verification: python -m pytest tests/test_reviewer.py -q -> 16 passed; git diff --check clean; no network, no real subprocesses beyond injected fakes.

Closes #5 Review gate: the automated PR reviewer. No dependencies — built directly on main (docs-only baseline). Files (only the allowed set): - `gitea_saas/gitea_client.py` — Gitea API seam (urllib, token from env, never on a command line): `get_issue`, `get_pr` (head_sha + changed_files), `get_pr_diff`, `post_comment`. Real HTTP only when a token exists; tests use an in-memory fake. - `gitea_saas/reviewer.py` — `Reviewer(repo_client, runner)` with `review_pr(repo, pr, linked_issue, repo_dir) -> Verdict` and `post_verdict(...)` (posts `[review-gate] VERDICT: ...` comment with the acceptance checklist + findings). - `tests/test_reviewer.py` — 16 hermetic tests. Pipeline per review: fetch PR head sha / files / diff -> parse acceptance checklist (`- [ ]`/`- [x]`, or `## Acceptance` section) -> parse **Files allowed** scope -> run `python -m pytest -q` (REVIEW_TEST_TIMEOUT_S, default 300) -> optional build cmd (REVIEW_BUILD_CMD) -> `git diff --check` -> findings pass (deterministic secret/TODO scan by default; configurable LLM pass validated so unquoted findings are dropped) -> verdict. Verdict rules: tests non-zero -> BOUNCE (unless `allow_test_failure` in the issue body); timeout -> BOUNCE with `tests.exit = "timed_out"`; scope violation -> BOUNCE; blocking finding -> BOUNCE; empty acceptance -> NEEDS_GUIDANCE (never silent-approve); else APPROVE. Fail-closed guarantees: - Findings must quote the diff (`evidence_quote`), redacting leaked secret values; the no-LLM default is fully deterministic. - No merge call anywhere in reviewer.py (AST-checked by a test); the posted comment only disclaims merging — "No merge performed by the review gate (policy/merge is issue #7)". - Test grounding: approve-check, bounce-on-tests, bounce-on-scope, needs-guidance-on-no-acceptance, allow_test_failure override, timeout -> BOUNCE (Runner timeout path tested hermetically via injected command runner), secret/TODO findings quoting the diff, build cmd + build-failure bounce, diff-hygiene blocking finding, verdict shape, no-merge AST scan. Verification: `python -m pytest tests/test_reviewer.py -q` -> 16 passed; `git diff --check` clean; no network, no real subprocesses beyond injected fakes.
pbwraith added 1 commit 2026-08-25 11:31:26 +02:00 
Reviewer(repo_client, runner) with review_pr() -> Verdict and
post_verdict() posting [review-gate] comments. Parses acceptance
checklists + Files-allowed scope from the linked issue; runs pytest,
optional build cmd (REVIEW_BUILD_CMD), git diff --check with timeouts
(REVIEW_TEST_TIMEOUT_S, default 300); secret-pattern scan (sk_live_,
AKIA, gh tokens, private keys, assigned credentials) quoting each added
diff line as evidence (values redacted); optional configurable LLM pass
that is validated — unquoted findings dropped. Verdict rules: failing
tests -> BOUNCE (unless allow_test_failure in issue body); timeout ->
BOUNCE; scope violation -> BOUNCE; blocking finding -> BOUNCE; empty
acceptance -> NEEDS_GUIDANCE (never silent-approve); else APPROVE.
FAIL-CLOSED: no merge call anywhere. gitea_client.py adds the Gitea
seam (env token, urllib, mockable). 16 hermetic tests, no network, no
real subprocesses.
pbwraith commented 2026-08-25 11:31:57 +02:00
Author
Owner
Copy Link

[worker-lane3] Implemented on branch issue-5 (base: main, no dependencies); PR #12 open for review (Closes #5).

Commit: df72b39 — "gitea-saas-core#5: review gate — automated PR reviewer (acceptance parse, tests, verdicts)"

Test evidence: python -m pytest tests/test_reviewer.py -q → 16 passed; git diff --check clean; hermetic (no network, no real subprocesses — FakeGiteaClient + FakeRunner seams, Runner timeout path tested via injected command runner). Acceptance coverage: APPROVE on green PR with comment checklist; BOUNCE on failing tests (evidence in verdict); BOUNCE with scope_violation outside Files allowed; NEEDS_GUIDANCE on issue without acceptance (never silent-approve); findings quote the diff (secret values redacted, lines anchored to real added lines); no merge call anywhere (AST-checked); timeout → BOUNCE with tests.exit="timed_out". Verdict rules + config via REVIEW_BUILD_CMD / REVIEW_TEST_TIMEOUT_S / REVIEW_SECRET_SCAN (+ optional llm callable, validated).

[worker-lane3] Implemented on branch `issue-5` (base: main, no dependencies); PR #12 open for review (Closes #5). Commit: `df72b39` — "gitea-saas-core#5: review gate — automated PR reviewer (acceptance parse, tests, verdicts)" Test evidence: `python -m pytest tests/test_reviewer.py -q` → 16 passed; `git diff --check` clean; hermetic (no network, no real subprocesses — FakeGiteaClient + FakeRunner seams, Runner timeout path tested via injected command runner). Acceptance coverage: APPROVE on green PR with comment checklist; BOUNCE on failing tests (evidence in verdict); BOUNCE with scope_violation outside Files allowed; NEEDS_GUIDANCE on issue without acceptance (never silent-approve); findings quote the diff (secret values redacted, lines anchored to real added lines); no merge call anywhere (AST-checked); timeout → BOUNCE with tests.exit="timed_out". Verdict rules + config via REVIEW_BUILD_CMD / REVIEW_TEST_TIMEOUT_S / REVIEW_SECRET_SCAN (+ optional llm callable, validated).
pbwraith commented 2026-08-25 12:40:18 +02:00
Author
Owner
Copy Link

Not merged — escalated to the owner, no defect found.

Verified so the owner reviews a known-good PR rather than an unknown one.
Rebased on the current main (with #1 merged): 0 conflicts, 42 passed.

I looked specifically at the credential handling, since that is what puts this
on the escalation list:

  • gitea_client.py:24 — token comes from the GITEA_TOKEN env var or an
    explicit argument, defaulting to "".
  • :30-37 — it is used only as an Authorization: token ... header on a
    urllib request. It is never logged, never interpolated into a message, and
    never placed on a command line, which matches the module docstring's claim.
  • An empty token fails closed in the safe direction: the request is simply
    rejected by the server. There is no path where a missing credential grants
    more access than a present one — which is exactly the failure mode its sibling
    !11 does have, so it is worth stating that this one does not.

So this is sound as far as I can verify it. The only reason it is not merged is
the standing rule that this sweep does not merge changes touching auth or
credentials — that is a policy boundary, not a criticism of the code.

Deliberately leaving it unlabelled: bounced would queue it for rework, and
there is nothing here to rework.

Not merged — **escalated to the owner, no defect found.** Verified so the owner reviews a known-good PR rather than an unknown one. Rebased on the current `main` (with #1 merged): **0 conflicts, 42 passed**. I looked specifically at the credential handling, since that is what puts this on the escalation list: - `gitea_client.py:24` — `token` comes from the `GITEA_TOKEN` env var or an explicit argument, defaulting to `""`. - `:30-37` — it is used only as an `Authorization: token ...` header on a `urllib` request. It is never logged, never interpolated into a message, and never placed on a command line, which matches the module docstring's claim. - An empty token **fails closed** in the safe direction: the request is simply rejected by the server. There is no path where a missing credential grants more access than a present one — which is exactly the failure mode its sibling !11 does have, so it is worth stating that this one does not. So this is sound as far as I can verify it. The only reason it is not merged is the standing rule that this sweep does not merge changes touching auth or credentials — that is a policy boundary, not a criticism of the code. Deliberately leaving it unlabelled: `bounced` would queue it for rework, and there is nothing here to rework.
pbwraith commented 2026-08-25 13:25:33 +02:00
Author
Owner
Copy Link

Superseded by !15 -- recommend closing rather than merging

git merge-base --is-ancestor origin/issue-5 origin/issue-6 returns true, and
the gitea_saas/gitea_client.py blobs on the two branches hash identically
(9698050b6f2ebcb4). Merging !15 therefore lands this PR's work in full.

Measured directly: applying !15 first takes the tree to 84 passed, and
applying !12 on top of that leaves it at 84 passed -- this PR contributes
zero additional tests once !15 is in.

Nothing here is wrong; it is just already contained in !15. Flagging it so
nobody spends a review cycle on it twice, and so it is not merged as a
third source of gitea_client.py.

Verified composition of all five non-bounced PRs (measured, not inferred)

Every one of these reports mergeable=true; that flag is computed against
main in isolation and says nothing about whether they coexist. Merged for
real into a throwaway worktree off origin/main (59c3d03), in this order:

baseline main                       26 passed
+ !11 (issue-3, stripe)   clean     42 passed   (+16)
+ !18 (issue-7, delivery) clean     53 passed   (+11)
+ !15 (issue-6, policy)   clean     84 passed   (+31)
+ !12 (issue-5, reviewer) clean     84 passed   (+0)   <- adds nothing; see below
+ !17 (issue-4, intake)   CONFLICT  gitea_saas/gitea_client.py

Resolving that one conflict by taking !17's client and re-running:

101 passed, 0 failed, 0 conflict markers

26 + 16 + 11 + 31 + 0 + 17 = 101 exactly, so nothing was lost in the resolution.

The conflict, and why taking !17's side is the right resolution

Three PRs each add gitea_saas/gitea_client.py as a new file. !12 and !15
ship a byte-identical 69-line version (same sha256); !17 ships a divergent
162-line one. !17's is a true superset by method name --

issue-5/6 (69L) : get_issue  get_pr  get_pr_diff  post_comment
issue-4  (162L) : get_issue  get_pr  get_pr_diff  post_comment
                  + repo_exists  create_org_repo  list_labels  label_id  create_issue

-- and the only client calls in the dependent modules are
get_pr / get_pr_diff / post_comment (reviewer.py) and
create_org_repo / create_issue (intake_factory.py), all present in !17's.
Taking !17's version and running the combined suite gives the 101 above, so the
resolution is verified rather than assumed.

!15 subsumes !12

git merge-base --is-ancestor origin/issue-5 origin/issue-6 -> true, and their
gitea_client.py blobs hash identically. Merging !15 lands !12 in full, which
is why !12 contributes +0 tests when applied afterwards. !12 should be
closed as subsumed rather than merged separately, unless it is meant to land
on its own.

Not merging this one -- owner call

This sweep does not merge changes that touch auth or credentials, and this repo
is a billing service: every open PR here handles either a payment secret
(STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET), a Gitea API token
(GITEA_TOKEN), or an API key that guards a ledger debit (SAAS_API_KEY).
That is a deliberate hold on the merge decision, not a finding against this
PR -- the verification above is green and the landing order is worked out, so
this should be a short decision rather than a re-derivation.

## Superseded by !15 -- recommend closing rather than merging `git merge-base --is-ancestor origin/issue-5 origin/issue-6` returns true, and the `gitea_saas/gitea_client.py` blobs on the two branches hash identically (`9698050b6f2ebcb4`). Merging !15 therefore lands this PR's work in full. Measured directly: applying !15 first takes the tree to **84 passed**, and applying !12 on top of that leaves it at **84 passed** -- this PR contributes zero additional tests once !15 is in. Nothing here is wrong; it is just already contained in !15. Flagging it so nobody spends a review cycle on it twice, and so it is not merged as a third source of `gitea_client.py`. ### Verified composition of all five non-bounced PRs (measured, not inferred) Every one of these reports `mergeable=true`; that flag is computed against `main` in isolation and says nothing about whether they coexist. Merged for real into a throwaway worktree off `origin/main` (`59c3d03`), in this order: ``` baseline main 26 passed + !11 (issue-3, stripe) clean 42 passed (+16) + !18 (issue-7, delivery) clean 53 passed (+11) + !15 (issue-6, policy) clean 84 passed (+31) + !12 (issue-5, reviewer) clean 84 passed (+0) <- adds nothing; see below + !17 (issue-4, intake) CONFLICT gitea_saas/gitea_client.py ``` Resolving that one conflict by taking !17's client and re-running: ``` 101 passed, 0 failed, 0 conflict markers ``` 26 + 16 + 11 + 31 + 0 + 17 = 101 exactly, so nothing was lost in the resolution. ### The conflict, and why taking !17's side is the right resolution Three PRs each **add** `gitea_saas/gitea_client.py` as a new file. !12 and !15 ship a byte-identical 69-line version (same sha256); !17 ships a divergent 162-line one. !17's is a true superset by method name -- ``` issue-5/6 (69L) : get_issue get_pr get_pr_diff post_comment issue-4 (162L) : get_issue get_pr get_pr_diff post_comment + repo_exists create_org_repo list_labels label_id create_issue ``` -- and the only client calls in the dependent modules are `get_pr` / `get_pr_diff` / `post_comment` (reviewer.py) and `create_org_repo` / `create_issue` (intake_factory.py), all present in !17's. Taking !17's version and running the combined suite gives the 101 above, so the resolution is verified rather than assumed. ### !15 subsumes !12 `git merge-base --is-ancestor origin/issue-5 origin/issue-6` -> true, and their `gitea_client.py` blobs hash identically. Merging !15 lands !12 in full, which is why !12 contributes **+0 tests** when applied afterwards. !12 should be closed as subsumed rather than merged separately, unless it is meant to land on its own. ### Not merging this one -- owner call This sweep does not merge changes that touch auth or credentials, and this repo is a billing service: every open PR here handles either a payment secret (`STRIPE_SECRET_KEY`, `STRIPE_WEBHOOK_SECRET`), a Gitea API token (`GITEA_TOKEN`), or an API key that guards a ledger debit (`SAAS_API_KEY`). That is a deliberate hold on the merge decision, **not** a finding against this PR -- the verification above is green and the landing order is worked out, so this should be a short decision rather than a re-derivation.
pbwraith commented 2026-08-25 17:27:40 +02:00
Author
Owner
Copy Link

Bounced: gitea_saas/gitea_client.py collides with three other PRs

This branch is clean against main on its own, but it does not coexist with its siblings. Merging every open branch into one worktree off main:

!! issue-5   CONFLICT  gitea_saas/gitea_client.py

Four PRs create that file: !17 (issue-4), !12 (this one), !15 (issue-6), !20 (issue-8). Gitea reports mergeable=true for all four because the file does not exist on main yet -- each one creates it -- so the flag cannot see this.

The fix here is a deletion

I recorded the full analysis and the ownership decision on #21: !17's version is canonical. Your copy is a strict capability subset of it.

  • Your file is byte-identical to !15's (empty diff between the blobs).
  • This branch's production code calls exactly get_pr, get_pr_diff and post_comment (AST pass over everything except gitea_client.py). All three exist on !17's client.
  • I substituted !17's client into this branch and ran your suite: 42 passed, identical to the 42 you get with your own copy.

So: delete gitea_saas/gitea_client.py from this branch and keep reviewer.py + test_reviewer.py. Nothing else changes and no capability is lost -- that is measured, not assumed.

Sequencing: !17 has to land first (it carries the canonical file). If you would rather not wait, deleting the file now still leaves this branch correct-on-merge, it just will not run standalone until !17 is in.

## Bounced: `gitea_saas/gitea_client.py` collides with three other PRs This branch is clean against `main` on its own, but it does not coexist with its siblings. Merging every open branch into one worktree off `main`: ``` !! issue-5 CONFLICT gitea_saas/gitea_client.py ``` Four PRs create that file: !17 (issue-4), !12 (this one), !15 (issue-6), !20 (issue-8). Gitea reports `mergeable=true` for all four because the file does not exist on `main` yet -- each one *creates* it -- so the flag cannot see this. ### The fix here is a deletion I recorded the full analysis and the ownership decision on #21: **!17's version is canonical.** Your copy is a strict capability subset of it. - Your file is **byte-identical** to !15's (empty `diff` between the blobs). - This branch's production code calls exactly `get_pr`, `get_pr_diff` and `post_comment` (AST pass over everything except `gitea_client.py`). All three exist on !17's client. - I substituted !17's client into this branch and ran your suite: **42 passed**, identical to the 42 you get with your own copy. So: **delete `gitea_saas/gitea_client.py` from this branch** and keep `reviewer.py` + `test_reviewer.py`. Nothing else changes and no capability is lost -- that is measured, not assumed. Sequencing: !17 has to land first (it carries the canonical file). If you would rather not wait, deleting the file now still leaves this branch correct-on-merge, it just will not run standalone until !17 is in.
pbwraith added the
bounced
 label 2026-08-25 17:27:41 +02:00
pbwraith added 1 commit 2026-08-25 17:43:49 +02:00
pbwraith removed the
bounced
 label 2026-08-25 17:45:14 +02:00
pbwraith commented 2026-08-25 17:45:15 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 1]

Review finding (2026-08-25 owner bounce on !12): gitea_saas/gitea_client.py collides with !17/!15/!20 — four PRs create the same file. Per the #21 ownership decision, !17's version is canonical; this branch's copy is byte-identical to !15's and a capability subset of !17's. Fix = deletion.

Fix applied (commit 5e5827c on issue-5): git rm gitea_saas/gitea_client.py — 69 lines removed. Production code here calls exactly get_pr / get_pr_diff / post_comment (all present on !17's client), and reviewer.py only mentions gitea_client.GiteaClient in a docstring — no import. Nothing else changed.

Finding-by-finding:

  • delete duplicate gitea_saas/gitea_client.py -> done, file removed from branch (PR diff now = gitea_saas/reviewer.py, tests/test_reviewer.py only)
  • keep reviewer.py + tests/test_reviewer.py unchanged -> done (zero edits to either)

Test evidence: python -m pytest -q on issue-5 standalone -> 16 passed (unlike the predicted interim state, nothing imports the deleted module, so the branch runs standalone even before !17 lands); git diff --check clean. Verified post-push: PR !12 state=open, merged=false, head=issue-5@5e5827c, mergeable=true.

Sequencing per the bounce: !17 (canonical client) still must land first; this branch is now correct-on-merge and carries zero additional copies of the file.

[worker-bounce-fix attempt 1] Review finding (2026-08-25 owner bounce on !12): `gitea_saas/gitea_client.py` collides with !17/!15/!20 — four PRs create the same file. Per the #21 ownership decision, !17's version is canonical; this branch's copy is byte-identical to !15's and a capability subset of !17's. Fix = deletion. **Fix applied (commit `5e5827c` on `issue-5`):** `git rm gitea_saas/gitea_client.py` — 69 lines removed. Production code here calls exactly `get_pr` / `get_pr_diff` / `post_comment` (all present on !17's client), and `reviewer.py` only mentions `gitea_client.GiteaClient` in a docstring — no import. Nothing else changed. **Finding-by-finding:** - delete duplicate `gitea_saas/gitea_client.py` -> done, file removed from branch (PR diff now = `gitea_saas/reviewer.py`, `tests/test_reviewer.py` only) - keep `reviewer.py` + `tests/test_reviewer.py` unchanged -> done (zero edits to either) **Test evidence:** `python -m pytest -q` on `issue-5` standalone -> **16 passed** (unlike the predicted interim state, nothing imports the deleted module, so the branch runs standalone even before !17 lands); `git diff --check` clean. Verified post-push: PR !12 state=open, merged=false, head=`issue-5`@`5e5827c`, mergeable=true. Sequencing per the bounce: !17 (canonical client) still must land first; this branch is now correct-on-merge and carries zero additional copies of the file.
pbwraith commented 2026-08-25 18:13:58 +02:00
Author
Owner
Copy Link

Merged — the bounce finding is closed, verified against the merge result

The original bounce is resolved

This PR was bounced because it created a fourth gitea_saas/gitea_client.py
(#21). Confirmed the fix landed: the branch's file list is now exactly
gitea_saas/reviewer.py + tests/test_reviewer.py, and the duplicate is gone.

Gate, on the real merge result

origin/main (c78f497) merged into origin/issue-5: 0 conflicts.

main control     29 passed
merge result     45 passed      (29 + 16, exact)

I re-measured the sequencing claim, and it does not hold

The fix comment says "!17 (canonical client) still must land first". I checked
that rather than inheriting it, and this branch does not depend on !17:

git grep gitea_client  ->  reviewer.py:10   (docstring only)
                           tests/*          (FakeGiteaClient fixture names)
                           ARCHITECTURE.md  (layout table)

There is no import. repo_client is a duck-typed constructor seam, which is
why the 16 tests pass standalone. So merging this now is safe, and it strictly
reduces !17's conflict surface rather than adding to it — !17 was the PR that
conflicted in #21's measured landing sequence, precisely because !12/!15 carried
their own copies.

Mutation-tested the load-bearing safety property

This gate posts test output into public PR comments, so the secret redaction is
the part that actually matters. I broke it and confirmed the suite notices:

baseline                                        16 passed
_redact returns text unchanged (no redaction)    1 failed, 15 passed
restored                                        16 passed

with the anchor and on-disk mutation both asserted before running, so this is
not a no-op mutation reporting a false pass.

The patterns are also not prefix-only — alongside sk_live_, AKIA, gh[pousr]_
and PEM blocks there is a generic
(?i)(api[_-]?key|secret|token|password)\s*[:=]\s*"..." rule with a 12-char
minimum. Prefix-only masking is a known way to leak keys here, so the generic
rule is the right shape.

Two observations, neither a merge blocker

  1. Runner defaults to subprocess.run(command, shell=True). I traced
    build_cmd to os.environ.get("REVIEW_BUILD_CMD") — operator-controlled,
    not read from the client repo — so this is not client-driven command
    injection. Running the client's pytest is arbitrary code execution by
    design, as in any CI; flagging it as the product premise, not a defect here.
  2. reviewer.py:10 points at gitea_client.GiteaClient, which will not exist
    on main until !17 lands. That is a forward reference to the module #4 now
    owns, so it is accurate as intent — but it is a docstring describing another
    component, which is a shape that has been wrong here before. Worth a second
    look when !17 lands.
## Merged — the bounce finding is closed, verified against the merge result ### The original bounce is resolved This PR was bounced because it created a fourth `gitea_saas/gitea_client.py` (#21). Confirmed the fix landed: the branch's file list is now exactly `gitea_saas/reviewer.py` + `tests/test_reviewer.py`, and the duplicate is gone. ### Gate, on the real merge result `origin/main` (`c78f497`) merged into `origin/issue-5`: **0 conflicts**. ``` main control 29 passed merge result 45 passed (29 + 16, exact) ``` ### I re-measured the sequencing claim, and it does not hold The fix comment says "!17 (canonical client) still must land first". I checked that rather than inheriting it, and **this branch does not depend on !17**: ``` git grep gitea_client -> reviewer.py:10 (docstring only) tests/* (FakeGiteaClient fixture names) ARCHITECTURE.md (layout table) ``` There is no `import`. `repo_client` is a duck-typed constructor seam, which is why the 16 tests pass standalone. So merging this now is safe, and it strictly *reduces* !17's conflict surface rather than adding to it — !17 was the PR that conflicted in #21's measured landing sequence, precisely because !12/!15 carried their own copies. ### Mutation-tested the load-bearing safety property This gate posts test output into public PR comments, so the secret redaction is the part that actually matters. I broke it and confirmed the suite notices: ``` baseline 16 passed _redact returns text unchanged (no redaction) 1 failed, 15 passed restored 16 passed ``` with the anchor and on-disk mutation both asserted before running, so this is not a no-op mutation reporting a false pass. The patterns are also not prefix-only — alongside `sk_live_`, `AKIA`, `gh[pousr]_` and PEM blocks there is a generic `(?i)(api[_-]?key|secret|token|password)\s*[:=]\s*"..."` rule with a 12-char minimum. Prefix-only masking is a known way to leak keys here, so the generic rule is the right shape. ### Two observations, neither a merge blocker 1. `Runner` defaults to `subprocess.run(command, shell=True)`. I traced `build_cmd` to `os.environ.get("REVIEW_BUILD_CMD")` — operator-controlled, **not** read from the client repo — so this is not client-driven command injection. Running the client's `pytest` is arbitrary code execution by design, as in any CI; flagging it as the product premise, not a defect here. 2. `reviewer.py:10` points at `gitea_client.GiteaClient`, which will not exist on `main` until !17 lands. That is a forward reference to the module #4 now owns, so it is accurate as intent — but it is a docstring describing another component, which is a shape that has been wrong here before. Worth a second look when !17 lands.
pbwraith merged commit bcf03bd9cc into main 2026-08-25 18:14:18 +02:00
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#12
No description provided.
Delete Branch "issue-5"

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?