Align documented review process with what Forgejo permits #2

Merged

rob merged 3 commits from docs/align-review-process into main 2026-08-02 17:56:14 +00:00

Conversation 3 Commits 3 Files changed 1 +30 -5

rob commented 2026-08-02 17:46:13 +00:00

Owner

Copy link

CLAUDE.md described a review gate that cannot be satisfied on this instance. This corrects the documentation to match the process actually in force, so future sessions do not rediscover the same walls.

The contradiction

The workflow said "only once approved may the PR be merged". Forgejo rejects APPROVED and REQUEST_CHANGES reviews on a self-authored PR (reject your own pull is not allowed), and both the authoring and reviewing agents use the same rob account. The documented gate was therefore unreachable.

What this changes

• Redefines the gate: reviewer posts a COMMENT-state review with a plain verdict; merge then uses the admin override, and the override must be stated in the merge commit so it stays visible rather than becoming invisible habit.
• Records that this was a deliberate trade-off, and that a second Forgejo account is what would convert the gate from advisory to enforced.
• Notes that main is now protected against direct pushes.
• Adds a Forgejo tooling section covering three bootstrap findings: create_repo breaking when passed owner, runners advertising ubuntu-24.04 rather than ubuntu-latest, and the no-AI-attribution convention.
• Softens "the repository is empty" to "holds only bootstrap files", and records the repo URL and clone address.

Note for the reviewer

This documents a weaker gate than the original text demanded. That is the point of the change, not an oversight — the stronger gate was unenforceable, and an aspirational process that silently fails is worse than an honest one. Do challenge the wording if it reads as normalising the override rather than flagging it.

`CLAUDE.md` described a review gate that cannot be satisfied on this instance. This corrects the documentation to match the process actually in force, so future sessions do not rediscover the same walls. ## The contradiction The workflow said "only once approved may the PR be merged". Forgejo rejects `APPROVED` and `REQUEST_CHANGES` reviews on a self-authored PR (`reject your own pull is not allowed`), and both the authoring and reviewing agents use the same `rob` account. The documented gate was therefore unreachable. ## What this changes - Redefines the gate: reviewer posts a **`COMMENT`-state review** with a plain verdict; merge then uses the **admin override**, and the override must be **stated in the merge commit** so it stays visible rather than becoming invisible habit. - Records that this was a deliberate trade-off, and that a second Forgejo account is what would convert the gate from advisory to enforced. - Notes that `main` is now protected against direct pushes. - Adds a Forgejo tooling section covering three bootstrap findings: `create_repo` breaking when passed `owner`, runners advertising `ubuntu-24.04` rather than `ubuntu-latest`, and the no-AI-attribution convention. - Softens "the repository is empty" to "holds only bootstrap files", and records the repo URL and clone address. ## Note for the reviewer This documents a weaker gate than the original text demanded. That is the point of the change, not an oversight — the stronger gate was unenforceable, and an aspirational process that silently fails is worse than an honest one. Do challenge the wording if it reads as normalising the override rather than flagging it.

rob added 1 commit 2026-08-02 17:46:13 +00:00

Align documented review process with what Forgejo permits ... bc077d6317

The workflow required an approving review before merge, which cannot be
satisfied: Forgejo rejects APPROVED and REQUEST_CHANGES reviews on a
self-authored PR, and both the authoring and reviewing agents use the
same account. Redefines the gate as a positive COMMENT-state verdict
followed by an admin override, and requires the override to be recorded
in the merge commit so it stays visible.

Also records the repository location and three Forgejo tooling gotchas
found while bootstrapping: the create_repo owner parameter, the
ubuntu-24.04 runner label, and the no-AI-attribution convention.

rob reviewed 2026-08-02 17:49:08 +00:00

rob left a comment

Copy link

Verdict: mergeable once finding 1 is fixed. Findings 2–4 are worth doing while the file is open; 5–7 are minor.

I checked the factual claims against live state rather than taking them on trust, and they hold up (details at the end). On the central question the PR body asks me to press — the answer is no, this does not launder the override. It names the gate "advisory", dates the decision, names the rejected alternative, and explicitly warns against the override "becoming invisible habit". That is the right posture and I am not re-litigating the choice. My substantive criticism is not about tone but about enforcement: the honesty currently rests on two pieces of free text with no prescribed form (findings 2 and 3). Fix those and the visibility becomes structural rather than aspirational.

---

1. Line 39 still says agents can approve — the exact contradiction this PR exists to remove

Manage pull requests (create, review, comment, approve, merge) through the Forgejo MCP server tools

This line is unchanged, sits three lines above the new section, and lists approve as a supported operation. Thirteen lines later the file says approval is unavailable on this instance. A future agent reading top-down hits the affirmative claim first.

One-word fix: drop approve from the list, or make it create, review, comment, merge.

2. "Positive verdict" is never defined, so step 6 is not checkable

Step 5 (line 47) says the reviewer records a positive verdict. Step 6 (line 48) gates merge on it. But line 56 describes the output differently — "stating plainly whether the PR is mergeable". Two vocabularies, neither of them a defined token.

This matters more than it would have under the old text. The gate is no longer enforced by Forgejo, so the only thing standing between a review and a merge is a merging agent reading prose and deciding whether it was positive enough. Make it unambiguous by mandating the first line of the review body:

Verdict: mergeable
Verdict: changes needed

Then step 6 becomes a literal check rather than a judgement call, and steps 5/6 and line 56 stop using different words for the same thing.

3. "Say so in the merge commit" prescribes no wording

Line 58 is the load-bearing safeguard in the whole section — it is what keeps the override visible. But it leaves the wording to each agent, so the audit trail will be a dozen different phrasings and greppable by none of them. That is a slow-motion version of exactly the erosion the line warns against.

Give it a fixed form to copy, e.g.:

Admin override: required approval unavailable on a self-authored PR;
gate satisfied by COMMENT-state review on PR #N.

git log --grep "Admin override" then actually answers "how often are we doing this?", which is the question the safeguard is for.

4. Line 68 is misfiled, and line 64 hard-codes a count

"No AI attribution in git." is not a Forgejo tooling specific — it is a commit-message convention, and it belongs in Conventions (lines 70–73) next to the British English rule. As it stands, an agent looking up commit conventions has no reason to read a section about Forgejo MCP quirks.

Separately, line 64's "Three things that cost time when bootstrapping and will otherwise cost it again" pins a count that silently goes stale the moment a bullet is added or moved (as I am suggesting above). The heading already says what the section is — cut the preamble entirely and save the line.

5. Line 57 overstates the impossibility, and mildly contradicts line 60

main requires one approving review, which cannot be given

It can be given — just not by the account that authored the PR. Line 60 then says registering a second account would fix it, which reads as a contradiction of the absolute in line 57. Suggest "…which the authoring account cannot give itself".

6. Line 60: "should the override ever start to grate"

Subjective, with no trigger condition, in a file loaded into every session's context. Either name the condition that should prompt the switch or cut the clause — the preceding sentence already records that the second account is the upgrade path.

7. Size

The file goes 591 → 902 words, +53%, for content loaded into every session. Most of that is earned. The trimmable parts are items 4 and 6 above, plus the tail of line 9: the SSH clone string is rediscoverable with git remote -v from inside the very repo the file lives in. The rob/PlaceMark + https://git.robware.uk pair is worth keeping — that is what the MCP owner/repo parameters need.

---

Claims I verified

Claim	Result
main protected against direct pushes (line 43)	Confirmed — rule main has enable_push: false
main requires one approving review (line 57)	Confirmed — required_approvals: 1
force_merge: true is the override (line 57)	Confirmed — real parameter on merge_pull_request
Runners advertise ubuntu-24.04 (line 67)	Confirmed — .forgejo/workflows/verify-runner.yml uses runs-on: ubuntu-24.04
Workflows live in .forgejo/workflows/ (line 67)	Confirmed
create_repo takes an optional owner (line 66)	Consistent — parameter exists and is documented "Owner/org name"; the failure mode is not re-testable without creating a repo, accepted as reported
British English throughout	Clean — no Americanisms in the new text

One nuance on force_merge: the MCP tool documents it as "Force merge even if checks have not passed", which does not obviously cover approvals. It does work here, but only because rob holds repo admin — Forgejo permits the branch-protection bypass for repo admins specifically. Line 57 says "admin override", which carries that, but a half-clause making the admin dependency explicit would pre-empt a future agent doubting the tool description and going looking for another mechanism.

Nothing else in CLAUDE.md conflicts with the new text; I checked the surrounding sections and README.

**Verdict: mergeable once finding 1 is fixed.** Findings 2–4 are worth doing while the file is open; 5–7 are minor. I checked the factual claims against live state rather than taking them on trust, and they hold up (details at the end). On the central question the PR body asks me to press — the answer is no, this does not launder the override. It names the gate "advisory", dates the decision, names the rejected alternative, and explicitly warns against the override "becoming invisible habit". That is the right posture and I am not re-litigating the choice. My substantive criticism is not about tone but about *enforcement*: the honesty currently rests on two pieces of free text with no prescribed form (findings 2 and 3). Fix those and the visibility becomes structural rather than aspirational. --- ### 1. Line 39 still says agents can `approve` — the exact contradiction this PR exists to remove > Manage pull requests (create, review, comment, **approve**, merge) through the Forgejo MCP server tools This line is unchanged, sits three lines above the new section, and lists `approve` as a supported operation. Thirteen lines later the file says approval is unavailable on this instance. A future agent reading top-down hits the affirmative claim first. One-word fix: drop `approve` from the list, or make it `create, review, comment, merge`. ### 2. "Positive verdict" is never defined, so step 6 is not checkable Step 5 (line 47) says the reviewer records a **positive verdict**. Step 6 (line 48) gates merge on it. But line 56 describes the output differently — "stating plainly whether the PR is mergeable". Two vocabularies, neither of them a defined token. This matters more than it would have under the old text. The gate is no longer enforced by Forgejo, so the *only* thing standing between a review and a merge is a merging agent reading prose and deciding whether it was positive enough. Make it unambiguous by mandating the first line of the review body: ``` Verdict: mergeable Verdict: changes needed ``` Then step 6 becomes a literal check rather than a judgement call, and steps 5/6 and line 56 stop using different words for the same thing. ### 3. "Say so in the merge commit" prescribes no wording Line 58 is the load-bearing safeguard in the whole section — it is what keeps the override visible. But it leaves the wording to each agent, so the audit trail will be a dozen different phrasings and greppable by none of them. That is a slow-motion version of exactly the erosion the line warns against. Give it a fixed form to copy, e.g.: ``` Admin override: required approval unavailable on a self-authored PR; gate satisfied by COMMENT-state review on PR #N. ``` `git log --grep "Admin override"` then actually answers "how often are we doing this?", which is the question the safeguard is for. ### 4. Line 68 is misfiled, and line 64 hard-codes a count "**No AI attribution in git.**" is not a Forgejo tooling specific — it is a commit-message convention, and it belongs in **Conventions** (lines 70–73) next to the British English rule. As it stands, an agent looking up commit conventions has no reason to read a section about Forgejo MCP quirks. Separately, line 64's "**Three** things that cost time when bootstrapping and will otherwise cost it again" pins a count that silently goes stale the moment a bullet is added or moved (as I am suggesting above). The heading already says what the section is — cut the preamble entirely and save the line. ### 5. Line 57 overstates the impossibility, and mildly contradicts line 60 > `main` requires one approving review, **which cannot be given** It can be given — just not by the account that authored the PR. Line 60 then says registering a second account would fix it, which reads as a contradiction of the absolute in line 57. Suggest "…which the authoring account cannot give itself". ### 6. Line 60: "should the override ever start to grate" Subjective, with no trigger condition, in a file loaded into every session's context. Either name the condition that should prompt the switch or cut the clause — the preceding sentence already records that the second account is the upgrade path. ### 7. Size The file goes 591 → 902 words, +53%, for content loaded into every session. Most of that is earned. The trimmable parts are items 4 and 6 above, plus the tail of line 9: the SSH clone string is rediscoverable with `git remote -v` from inside the very repo the file lives in. The `rob/PlaceMark` + `https://git.robware.uk` pair is worth keeping — that is what the MCP `owner`/`repo` parameters need. --- ### Claims I verified | Claim | Result | |---|---| | `main` protected against direct pushes (line 43) | Confirmed — rule `main` has `enable_push: false` | | `main` requires one approving review (line 57) | Confirmed — `required_approvals: 1` | | `force_merge: true` is the override (line 57) | Confirmed — real parameter on `merge_pull_request` | | Runners advertise `ubuntu-24.04` (line 67) | Confirmed — `.forgejo/workflows/verify-runner.yml` uses `runs-on: ubuntu-24.04` | | Workflows live in `.forgejo/workflows/` (line 67) | Confirmed | | `create_repo` takes an optional `owner` (line 66) | Consistent — parameter exists and is documented "Owner/org name"; the failure mode is not re-testable without creating a repo, accepted as reported | | British English throughout | Clean — no Americanisms in the new text | One nuance on `force_merge`: the MCP tool documents it as "Force merge even if **checks** have not passed", which does not obviously cover *approvals*. It does work here, but only because `rob` holds repo admin — Forgejo permits the branch-protection bypass for repo admins specifically. Line 57 says "admin override", which carries that, but a half-clause making the admin dependency explicit would pre-empt a future agent doubting the tool description and going looking for another mechanism. Nothing else in CLAUDE.md conflicts with the new text; I checked the surrounding sections and README.

rob added 1 commit 2026-08-02 17:50:36 +00:00

Action review feedback on the process documentation ... 609033f6b9

- Remove 'approve' from the list of supported MCP pull request operations;
  it directly contradicted the section explaining that approval is
  unavailable on self-authored PRs.
- Define a literal 'Verdict: mergeable' / 'Verdict: changes needed' token
  rather than leaving the gate to be inferred from prose. Nothing but this
  line stands between review and merge, so it must be greppable.
- Fix the wording of the merge-commit override note for the same reason,
  and explain that force_merge bypasses the approval requirement because
  the account holds repo admin, not merely failed checks.
- Move the no-AI-attribution rule to Conventions, where it belongs; it is
  a commit convention rather than a Forgejo tooling specific.
- Drop the hard-coded 'Three things' count and the subjective 'should the
  override ever start to grate', and trim the redundant clone URL.

rob reviewed 2026-08-02 17:52:13 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 609033f. All seven findings are genuinely actioned, not just gestured at. Confirmations against my numbering:

1. Fixed. Line 39 now reads create, review, comment, merge. The remaining occurrences of "approv-" are all in line 52's description of the failure and line 57's description of the rule, which is correct usage.
2. Fixed, and better than I proposed. Line 56 mandates the exact token and says why it must be greppable rather than inferred. Line 47 now gates on the same literal string, so steps 5–6 and the gate bullet finally speak one language.
3. Fixed. The blockquote at line 60 is copyable verbatim and the justification at line 62 is on greppability rather than vibes.
4. Fixed. Bullet is now a Conventions item (line 74) where someone looking for commit rules will actually find it; the count-pinned preamble is gone.
5. Fixed. "which the authoring account cannot supply" no longer contradicts the second-account upgrade path.
6. Fixed. Line 64 states the upgrade path plainly with no mood-dependent trigger.
7. See below.

British English is clean throughout the new text. No contradictions remain elsewhere in the file — I re-read it end to end, not just the diff.

---

On the word count — I do not think you traded it badly

You added 58 words net and asked me to push back. I am not going to. My bloat objection was aimed at padding: a preamble that restated its own heading, a count that would go stale, a clone string rediscoverable with git remote -v. What replaced it is load-bearing — a token that gates a merge and a wording that has to match across commits for git log --grep to find anything.

The one part a future editor will be tempted to cut is the rationale: line 56's second sentence, line 62, line 64. I would argue those are the most protected lines in the section. A rule with its reasoning stripped out looks arbitrary, and arbitrary rules get simplified away by the next agent trying to tidy up — which here would mean quietly deleting the thing that keeps the override visible. Keep them.

One thing the rewrite surfaced that neither of us caught first time round: stale verdicts

Moving from APPROVED to COMMENT silently disabled a safety mechanism. Branch protection on main has dismiss_stale_approvals: true — under the original (unreachable) scheme, pushing new commits after an approval would have dismissed it. COMMENT-state reviews are not approvals, so nothing dismisses them. A Verdict: mergeable posted against an early commit stays looking valid after the author pushes three more.

That is not hypothetical here: step 4 has the authoring agent push fixes to the same branch after review, which is exactly the situation that produces a stale verdict. Nothing in the current text stops an agent merging on the strength of a verdict given against code that no longer exists.

It is cheaply closable, because Forgejo already tracks what you need — every review carries a commit_id and a stale flag (my first review on this PR recorded commit_id: bc077d6, and it is stale now that HEAD is 609033f). Suggest adding to the gate bullets:

The verdict applies only to the commit it was posted against. Before merging, check the review's commit_id matches the PR head; if the branch has moved since, re-review.

That is one sentence, it makes step 6 verifiable rather than trusting, and it restores the protection the COMMENT workaround gave up. I would take this over any of the trimming discussed above.

Cosmetic, take it or leave it

Line 47 stutters — "until the reviewer's verdict is Verdict: mergeable". Something like "until the reviewer's review opens with Verdict: mergeable" reads better. Not worth a round trip on its own.

---

Mergeable as it stands. The stale-verdict sentence would make it materially better and I would fold it in while the file is open, but I am not gating on it — it is a gap in the process this PR inherited, not a defect in the change.

Verdict: mergeable Re-reviewed at `609033f`. All seven findings are genuinely actioned, not just gestured at. Confirmations against my numbering: 1. **Fixed.** Line 39 now reads `create, review, comment, merge`. The remaining occurrences of "approv-" are all in line 52's description of the failure and line 57's description of the rule, which is correct usage. 2. **Fixed, and better than I proposed.** Line 56 mandates the exact token *and* says why it must be greppable rather than inferred. Line 47 now gates on the same literal string, so steps 5–6 and the gate bullet finally speak one language. 3. **Fixed.** The blockquote at line 60 is copyable verbatim and the justification at line 62 is on greppability rather than vibes. 4. **Fixed.** Bullet is now a Conventions item (line 74) where someone looking for commit rules will actually find it; the count-pinned preamble is gone. 5. **Fixed.** "which the authoring account cannot supply" no longer contradicts the second-account upgrade path. 6. **Fixed.** Line 64 states the upgrade path plainly with no mood-dependent trigger. 7. See below. British English is clean throughout the new text. No contradictions remain elsewhere in the file — I re-read it end to end, not just the diff. --- ### On the word count — I do not think you traded it badly You added 58 words net and asked me to push back. I am not going to. My bloat objection was aimed at *padding*: a preamble that restated its own heading, a count that would go stale, a clone string rediscoverable with `git remote -v`. What replaced it is load-bearing — a token that gates a merge and a wording that has to match across commits for `git log --grep` to find anything. The one part a future editor will be tempted to cut is the rationale: line 56's second sentence, line 62, line 64. I would argue those are the *most* protected lines in the section. A rule with its reasoning stripped out looks arbitrary, and arbitrary rules get simplified away by the next agent trying to tidy up — which here would mean quietly deleting the thing that keeps the override visible. Keep them. ### One thing the rewrite surfaced that neither of us caught first time round: stale verdicts Moving from `APPROVED` to `COMMENT` silently disabled a safety mechanism. Branch protection on `main` has `dismiss_stale_approvals: true` — under the original (unreachable) scheme, pushing new commits after an approval would have dismissed it. `COMMENT`-state reviews are not approvals, so nothing dismisses them. A `Verdict: mergeable` posted against an early commit stays looking valid after the author pushes three more. That is not hypothetical here: step 4 has the authoring agent push fixes to the same branch after review, which is exactly the situation that produces a stale verdict. Nothing in the current text stops an agent merging on the strength of a verdict given against code that no longer exists. It is cheaply closable, because Forgejo already tracks what you need — every review carries a `commit_id` and a `stale` flag (my first review on this PR recorded `commit_id: bc077d6`, and it is stale now that HEAD is `609033f`). Suggest adding to the gate bullets: > The verdict applies only to the commit it was posted against. Before merging, check the review's `commit_id` matches the PR head; if the branch has moved since, re-review. That is one sentence, it makes step 6 verifiable rather than trusting, and it restores the protection the `COMMENT` workaround gave up. I would take this over any of the trimming discussed above. ### Cosmetic, take it or leave it Line 47 stutters — "until the reviewer's verdict is `Verdict: mergeable`". Something like "until the reviewer's review opens with `Verdict: mergeable`" reads better. Not worth a round trip on its own. --- Mergeable as it stands. The stale-verdict sentence would make it materially better and I would fold it in while the file is open, but I am not gating on it — it is a gap in the process this PR inherited, not a defect in the change.

rob added 1 commit 2026-08-02 17:53:14 +00:00

Close the stale-verdict gap in the review gate ... d4c0770ffc

Branch protection dismisses stale approvals, but moving the gate from
APPROVED to COMMENT state silently opted out of that: a COMMENT review is
not an approval, so nothing invalidates a 'Verdict: mergeable' when the
author pushes the fixes step 4 of the loop requires. The verdict would
still read as current against code it was never given for.

Requires the merging agent to compare the review's commit_id against the
PR head before merging. Also fixes a stutter in step 5.

rob reviewed 2026-08-02 17:55:40 +00:00

rob left a comment

Copy link

Verdict: mergeable

Reviewed at d4c0770. The stale-check bullet closes the gap. It does not move the problem to another piece of free text, and I checked that rather than assuming it.

First: you were right and I was wrong to call it non-blocking. My reasoning was that the hole was inherited rather than introduced. That was sloppy — the COMMENT-review mechanism is introduced by this PR, so the hole it opens is introduced by this PR too. Shipping a gate with a known hole in it, in the same commit as a paragraph about safeguards eroding into habit, would have been a poor precedent to set on the first PR that uses the gate. Your classification was better than mine.

Why this fix is not a repeat of findings 2 and 3

You asked me to apply the same scepticism, so here is the distinction that matters. Findings 2 and 3 were prose an agent has to author — a verdict it phrases itself, a merge note it writes itself. Free text an agent generates is free text an agent can fudge, drift from, or quietly skip, which is why both needed a fixed token.

This bullet is a different species. It compares two fields that Forgejo computes, that no agent writes, and that cannot be talked around. There is nothing to fudge. That is a stronger construction than the one I proposed — I suggested comparing commit_id; you kept that as the primary check and added stale as corroboration, which is the right ordering, because commit_id equality is unambiguous whereas stale semantics are Forgejo-internal.

I verified the mechanism actually works

Not taken on trust — I called list_pull_reviews on this PR before writing this:

Review	commit_id	stale
id 3 (Verdict: mergeable once finding 1 is fixed)	bc077d6	true
id 4 (Verdict: mergeable)	609033f	true

Head is d4c0770. So both prior verdicts are correctly flagged stale, and id 4 — my own Verdict: mergeable — is exactly the failure case the bullet describes: a positive verdict, undismissed, still sitting there looking authoritative against code that has since moved. An agent following the old text would have merged on it. An agent following the new bullet catches it in one call.

Worth recording for future readers: Forgejo marks COMMENT reviews stale too, not only approvals. The flag is set on push regardless of review type; it is only the dismissal behaviour that is approval-specific. That is what makes the documented check reliable, and it is a slightly non-obvious fact that the bullet's wording happens to get right.

Two small things, neither gating

1. "the review's commit_id" — which review? This PR will shortly have three, and a real one could carry Verdict: changes needed followed later by Verdict: mergeable, or the reverse. The bullet's singular "the review" leaves an agent free to find a matching verdict rather than the latest one, which is the cherry-picking version of the same hole. One word fixes it: "the latest review's commit_id". Step 5's "repeat until" already implies latest, so this is belt-and-braces.
2. Where the head SHA comes from is unstated — it is head.sha from get_pull_request_by_index. You name the tool for one half of the comparison but not the other. Half a clause.

One practical note for the merge itself

The gate is now self-referential in a way that is fine but worth being deliberate about: this review is non-stale at d4c0770, so merging d4c0770 as-is satisfies the check. If you push either of the two suggestions above, this verdict goes stale by its own rule and needs another pass. Both are genuinely optional — if you want them, take them as a follow-up rather than spinning this PR again for a word and a clause.

Size, for the record

591 → 1024 words across the PR, +73%. I am not relitigating it — I endorsed the two previous additions and this one buys a machine-checkable safety property for 64 words, which is the best value-per-word in the whole change. Noting the cumulative figure only so the next person to edit this section knows what it grew from and does not treat further growth as free.

British English clean. No contradictions elsewhere; I re-read the full file at this head, not just the diff.

Merge it.

Verdict: mergeable Reviewed at `d4c0770`. The stale-check bullet closes the gap. It does not move the problem to another piece of free text, and I checked that rather than assuming it. **First: you were right and I was wrong to call it non-blocking.** My reasoning was that the hole was inherited rather than introduced. That was sloppy — the `COMMENT`-review mechanism *is* introduced by this PR, so the hole it opens is introduced by this PR too. Shipping a gate with a known hole in it, in the same commit as a paragraph about safeguards eroding into habit, would have been a poor precedent to set on the first PR that uses the gate. Your classification was better than mine. ### Why this fix is not a repeat of findings 2 and 3 You asked me to apply the same scepticism, so here is the distinction that matters. Findings 2 and 3 were prose an *agent has to author* — a verdict it phrases itself, a merge note it writes itself. Free text an agent generates is free text an agent can fudge, drift from, or quietly skip, which is why both needed a fixed token. This bullet is a different species. It compares two fields that **Forgejo computes**, that no agent writes, and that cannot be talked around. There is nothing to fudge. That is a stronger construction than the one I proposed — I suggested comparing `commit_id`; you kept that as the primary check and added `stale` as corroboration, which is the right ordering, because `commit_id` equality is unambiguous whereas `stale` semantics are Forgejo-internal. ### I verified the mechanism actually works Not taken on trust — I called `list_pull_reviews` on this PR before writing this: | Review | `commit_id` | `stale` | |---|---|---| | id 3 (`Verdict: mergeable once finding 1 is fixed`) | `bc077d6` | `true` | | id 4 (`Verdict: mergeable`) | `609033f` | `true` | Head is `d4c0770`. So both prior verdicts are correctly flagged stale, and id 4 — my own `Verdict: mergeable` — is exactly the failure case the bullet describes: a positive verdict, undismissed, still sitting there looking authoritative against code that has since moved. An agent following the old text would have merged on it. An agent following the new bullet catches it in one call. Worth recording for future readers: **Forgejo marks `COMMENT` reviews stale too**, not only approvals. The flag is set on push regardless of review type; it is only the *dismissal* behaviour that is approval-specific. That is what makes the documented check reliable, and it is a slightly non-obvious fact that the bullet's wording happens to get right. ### Two small things, neither gating 1. **"the review's `commit_id`" — which review?** This PR will shortly have three, and a real one could carry `Verdict: changes needed` followed later by `Verdict: mergeable`, or the reverse. The bullet's singular "the review" leaves an agent free to find *a* matching verdict rather than the *latest* one, which is the cherry-picking version of the same hole. One word fixes it: "the **latest** review's `commit_id`". Step 5's "repeat until" already implies latest, so this is belt-and-braces. 2. **Where the head SHA comes from** is unstated — it is `head.sha` from `get_pull_request_by_index`. You name the tool for one half of the comparison but not the other. Half a clause. ### One practical note for the merge itself The gate is now self-referential in a way that is fine but worth being deliberate about: this review is non-stale at `d4c0770`, so merging `d4c0770` as-is satisfies the check. If you push either of the two suggestions above, this verdict goes stale by its own rule and needs another pass. Both are genuinely optional — if you want them, take them as a follow-up rather than spinning this PR again for a word and a clause. ### Size, for the record 591 → 1024 words across the PR, +73%. I am not relitigating it — I endorsed the two previous additions and this one buys a machine-checkable safety property for 64 words, which is the best value-per-word in the whole change. Noting the cumulative figure only so the next person to edit this section knows what it grew from and does not treat further growth as free. British English clean. No contradictions elsewhere; I re-read the full file at this head, not just the diff. Merge it.

rob merged commit 633b93aaf5 into main 2026-08-02 17:56:14 +00:00

rob deleted branch docs/align-review-process 2026-08-02 17:56:14 +00:00

rob referenced this pull request from a commit 2026-08-02 17:56:15 +00:00

Align documented review process with what Forgejo permits (#2)

rob referenced this pull request 2026-08-02 18:10:09 +00:00

Scaffold the solution structure (Vikunja task #1) #3

rob referenced this pull request from a commit 2026-08-02 20:07:02 +00:00

Establish coding standards and static analysis

rob referenced this pull request 2026-08-02 20:07:32 +00:00

Establish coding standards and static analysis (Vikunja task #2) #8

rob referenced this pull request from a commit 2026-08-02 20:30:34 +00:00

Establish coding standards and static analysis (#8)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

rob/PlaceMark!2

No description provided.