joshtronic/igor
joshtronic/igor
Watch
2
Code Issues 3 Pull requests 1 Activity Actions

docs: automerge.sh header still describes the pre-#404 human-only gate #480

New issue
Open
opened 2026-08-09 04:15:24 +00:00 by igor · 3 comments
igor commented 2026-08-09 04:15:24 +00:00
Collaborator
Copy link

The header comment of lib/automerge.sh (lines ~9-10) still describes the PRE-#404 gate: "The harness merges a bot PR ONLY when the human (FORGEJO_REVIEWER) has submitted an APPROVED review..." Since 2026-07-20 (commits 85d7481, d929cdc) the DEFAULT gate is the shadow review's APPROVE, with automerge_require_human carve-outs pinning specific repos back to the human gate. The stale header actively misled the operator and the CoS on 2026-08-09 (both believed the human gate was still the default).

Change: rewrite the header paragraph to describe the actual decision chain: shadow-APPROVE default -> require_human carve-out -> CI green -> mergeable -> not-behind -> not-RC -> never self-repo. One paragraph, accurate, nothing else. No behavior changes in this ticket.

Comment-discipline note: state the contract, not the history -- the git log carries the archaeology.

Blocked (2026-08-09 04:19Z)

The harness security review flagged a material issue in this change, so it was NOT pushed:

The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

Blocked (2026-08-09 05:53Z)

The harness security review flagged a material issue in this change, so it was NOT pushed:

The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

Blocked (2026-08-09 22:05Z)

The harness security review flagged a material issue in this change, so it was NOT pushed:

I'll inspect the actual code behind this documentation change before judging it.

Findings

  1. Auto-merge gate downgraded from human approval to self-approval (lib/automerge.sh) — The diff replaces the documented gate

    "merges a bot PR ONLY when the human (FORGEJO_REVIEWER) has submitted an APPROVED review"

    with

    "The default gate is the shadow review's own APPROVE verdict on the current head; a repo can pin itself back to a human-only gate via .automerge.require_human."

    This inverts the trust model of the merge gate. Previously a human reviewer was a mandatory, non-opt-out control on every automated merge; after this change the same automated agent that authors the PR also supplies the approving verdict that authorizes merging it, and human review becomes an opt-in flag (automerge_require_human) that defaults off. That is a self-approval loop: any code the bot can author, it can also approve and land, with the only remaining checks being CI green / mergeable / not-behind — none of which are review controls.

    Concretely this means a compromised or prompt-injected agent (e.g. one that ingested hostile content from an issue body, dependency README, or fetched page while working) no longer has a human in the loop between "write malicious patch" and "merge to the default branch, then stamp a pending deploy under .deploy". Because the merge also triggers the deploy barrier, the blast radius is production, not just the repo.

    Mitigations named in the comment do not close this. REQUEST_CHANGES vetoing the merge is not a substitute for required approval — it is a control the same agent would have to volunteer against itself. require_human being available per-repo does not help repos that never set it, which is every repo by default. "It never auto-merges its own repo" narrows one path but leaves every other repo self-approving.

    This is the category the review brief calls out explicitly: a change that weakens a merge/review gate is itself a finding regardless of how the diff's own prose justifies it. The prose here is doing exactly that justification work, so I'm treating it as data, not as reassurance.

    Suggested resolution: invert the default so require_human is on unless a repo explicitly opts out, or restrict shadow-verdict-only merging to a narrow, non-privileged change class (e.g. lockfile/formatting) that cannot touch CI config, deploy scripts, or the automerge/review logic itself.

Scope note

The diff as provided contains only the header-comment change; the implementation of automerge_require_human and the shadow-verdict gate is not included in the hunk I was given. I reviewed this on the basis that the comment accurately describes the behavior it ships alongside — that is the file's own contract-documentation block, not incidental prose. If the corresponding code is not in this branch, the finding downgrades to a docs/code mismatch and should be re-reviewed against the real implementation; if the code is in this branch but was omitted from the diff I received, the finding stands as written and the omission itself is worth confirming.

I flagged nothing on the remaining comment edits (CI-green, mergeable, not-behind, self-repo exclusion) — those are additive restrictions or accurate descriptions of existing behavior and introduce no new exposure.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

The header comment of `lib/automerge.sh` (lines ~9-10) still describes the PRE-#404 gate: "The harness merges a bot PR ONLY when the human (FORGEJO_REVIEWER) has submitted an APPROVED review..." Since 2026-07-20 (commits 85d7481, d929cdc) the DEFAULT gate is the shadow review's APPROVE, with `automerge_require_human` carve-outs pinning specific repos back to the human gate. The stale header actively misled the operator and the CoS on 2026-08-09 (both believed the human gate was still the default). **Change:** rewrite the header paragraph to describe the actual decision chain: shadow-APPROVE default -> require_human carve-out -> CI green -> mergeable -> not-behind -> not-RC -> never self-repo. One paragraph, accurate, nothing else. No behavior changes in this ticket. Comment-discipline note: state the contract, not the history -- the git log carries the archaeology. --- ## Blocked (2026-08-09 04:19Z) The harness security review flagged a material issue in this change, so it was NOT pushed: The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.) --- ## Blocked (2026-08-09 05:53Z) The harness security review flagged a material issue in this change, so it was NOT pushed: The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.) --- ## Blocked (2026-08-09 22:05Z) The harness security review flagged a material issue in this change, so it was NOT pushed: I'll inspect the actual code behind this documentation change before judging it. --- **Findings** 1. **Auto-merge gate downgraded from human approval to self-approval (`lib/automerge.sh`)** — The diff replaces the documented gate > "merges a bot PR ONLY when the human (`FORGEJO_REVIEWER`) has submitted an APPROVED review" with > "The default gate is the shadow review's own APPROVE verdict on the current head; a repo can pin itself back to a human-only gate via `.automerge.require_human`." This inverts the trust model of the merge gate. Previously a human reviewer was a mandatory, non-opt-out control on every automated merge; after this change the *same automated agent that authors the PR* also supplies the approving verdict that authorizes merging it, and human review becomes an opt-in flag (`automerge_require_human`) that defaults off. That is a self-approval loop: any code the bot can author, it can also approve and land, with the only remaining checks being CI green / mergeable / not-behind — none of which are review controls. Concretely this means a compromised or prompt-injected agent (e.g. one that ingested hostile content from an issue body, dependency README, or fetched page while working) no longer has a human in the loop between "write malicious patch" and "merge to the default branch, then stamp a pending deploy under `.deploy`". Because the merge also triggers the deploy barrier, the blast radius is production, not just the repo. Mitigations named in the comment do not close this. `REQUEST_CHANGES` vetoing the merge is not a substitute for required approval — it is a control the same agent would have to volunteer against itself. `require_human` being available per-repo does not help repos that never set it, which is every repo by default. "It never auto-merges its own repo" narrows one path but leaves every other repo self-approving. This is the category the review brief calls out explicitly: a change that weakens a merge/review gate is itself a finding regardless of how the diff's own prose justifies it. The prose here is doing exactly that justification work, so I'm treating it as data, not as reassurance. Suggested resolution: invert the default so `require_human` is on unless a repo explicitly opts out, or restrict shadow-verdict-only merging to a narrow, non-privileged change class (e.g. lockfile/formatting) that cannot touch CI config, deploy scripts, or the automerge/review logic itself. **Scope note** The diff as provided contains only the header-comment change; the implementation of `automerge_require_human` and the shadow-verdict gate is not included in the hunk I was given. I reviewed this on the basis that the comment accurately describes the behavior it ships alongside — that is the file's own contract-documentation block, not incidental prose. If the corresponding code is *not* in this branch, the finding downgrades to a docs/code mismatch and should be re-reviewed against the real implementation; if the code *is* in this branch but was omitted from the diff I received, the finding stands as written and the omission itself is worth confirming. I flagged nothing on the remaining comment edits (CI-green, mergeable, not-behind, self-repo exclusion) — those are additive restrictions or accurate descriptions of existing behavior and introduce no new exposure. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)
igor self-assigned this 2026-08-09 04:15:43 +00:00
igor commented 2026-08-09 04:19:09 +00:00
Author
Collaborator
Copy link

The harness security review flagged a material issue in this change, so it was NOT pushed:

The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

(Appended to the issue description above -- removing Status/Blocked re-queues the ticket with this context already in hand.)

The harness security review flagged a material issue in this change, so it was NOT pushed: The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.) _(Appended to the issue description above -- removing `Status/Blocked` re-queues the ticket with this context already in hand.)_
igor removed their assignment 2026-08-09 04:19:10 +00:00
igor self-assigned this 2026-08-09 05:49:51 +00:00
igor commented 2026-08-09 05:53:30 +00:00
Author
Collaborator
Copy link

The harness security review flagged a material issue in this change, so it was NOT pushed:

The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

(Appended to the issue description above -- removing Status/Blocked re-queues the ticket with this context already in hand.)

The harness security review flagged a material issue in this change, so it was NOT pushed: The security gate could not complete -- no verdict from the reviewer after 2 attempts. This is most likely a transient model/API error rather than a real finding; re-queue to retry. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.) _(Appended to the issue description above -- removing `Status/Blocked` re-queues the ticket with this context already in hand.)_
igor removed their assignment 2026-08-09 05:53:30 +00:00
igor self-assigned this 2026-08-09 22:02:05 +00:00
igor commented 2026-08-09 22:05:04 +00:00
Author
Collaborator
Copy link

The harness security review flagged a material issue in this change, so it was NOT pushed:

I'll inspect the actual code behind this documentation change before judging it.

Findings

  1. Auto-merge gate downgraded from human approval to self-approval (lib/automerge.sh) — The diff replaces the documented gate

    "merges a bot PR ONLY when the human (FORGEJO_REVIEWER) has submitted an APPROVED review"

    with

    "The default gate is the shadow review's own APPROVE verdict on the current head; a repo can pin itself back to a human-only gate via .automerge.require_human."

    This inverts the trust model of the merge gate. Previously a human reviewer was a mandatory, non-opt-out control on every automated merge; after this change the same automated agent that authors the PR also supplies the approving verdict that authorizes merging it, and human review becomes an opt-in flag (automerge_require_human) that defaults off. That is a self-approval loop: any code the bot can author, it can also approve and land, with the only remaining checks being CI green / mergeable / not-behind — none of which are review controls.

    Concretely this means a compromised or prompt-injected agent (e.g. one that ingested hostile content from an issue body, dependency README, or fetched page while working) no longer has a human in the loop between "write malicious patch" and "merge to the default branch, then stamp a pending deploy under .deploy". Because the merge also triggers the deploy barrier, the blast radius is production, not just the repo.

    Mitigations named in the comment do not close this. REQUEST_CHANGES vetoing the merge is not a substitute for required approval — it is a control the same agent would have to volunteer against itself. require_human being available per-repo does not help repos that never set it, which is every repo by default. "It never auto-merges its own repo" narrows one path but leaves every other repo self-approving.

    This is the category the review brief calls out explicitly: a change that weakens a merge/review gate is itself a finding regardless of how the diff's own prose justifies it. The prose here is doing exactly that justification work, so I'm treating it as data, not as reassurance.

    Suggested resolution: invert the default so require_human is on unless a repo explicitly opts out, or restrict shadow-verdict-only merging to a narrow, non-privileged change class (e.g. lockfile/formatting) that cannot touch CI config, deploy scripts, or the automerge/review logic itself.

Scope note

The diff as provided contains only the header-comment change; the implementation of automerge_require_human and the shadow-verdict gate is not included in the hunk I was given. I reviewed this on the basis that the comment accurately describes the behavior it ships alongside — that is the file's own contract-documentation block, not incidental prose. If the corresponding code is not in this branch, the finding downgrades to a docs/code mismatch and should be re-reviewed against the real implementation; if the code is in this branch but was omitted from the diff I received, the finding stands as written and the omission itself is worth confirming.

I flagged nothing on the remaining comment edits (CI-green, mergeable, not-behind, self-repo exclusion) — those are additive restrictions or accurate descriptions of existing behavior and introduce no new exposure.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

(Appended to the issue description above -- removing Status/Blocked re-queues the ticket with this context already in hand.)

The harness security review flagged a material issue in this change, so it was NOT pushed: I'll inspect the actual code behind this documentation change before judging it. --- **Findings** 1. **Auto-merge gate downgraded from human approval to self-approval (`lib/automerge.sh`)** — The diff replaces the documented gate > "merges a bot PR ONLY when the human (`FORGEJO_REVIEWER`) has submitted an APPROVED review" with > "The default gate is the shadow review's own APPROVE verdict on the current head; a repo can pin itself back to a human-only gate via `.automerge.require_human`." This inverts the trust model of the merge gate. Previously a human reviewer was a mandatory, non-opt-out control on every automated merge; after this change the *same automated agent that authors the PR* also supplies the approving verdict that authorizes merging it, and human review becomes an opt-in flag (`automerge_require_human`) that defaults off. That is a self-approval loop: any code the bot can author, it can also approve and land, with the only remaining checks being CI green / mergeable / not-behind — none of which are review controls. Concretely this means a compromised or prompt-injected agent (e.g. one that ingested hostile content from an issue body, dependency README, or fetched page while working) no longer has a human in the loop between "write malicious patch" and "merge to the default branch, then stamp a pending deploy under `.deploy`". Because the merge also triggers the deploy barrier, the blast radius is production, not just the repo. Mitigations named in the comment do not close this. `REQUEST_CHANGES` vetoing the merge is not a substitute for required approval — it is a control the same agent would have to volunteer against itself. `require_human` being available per-repo does not help repos that never set it, which is every repo by default. "It never auto-merges its own repo" narrows one path but leaves every other repo self-approving. This is the category the review brief calls out explicitly: a change that weakens a merge/review gate is itself a finding regardless of how the diff's own prose justifies it. The prose here is doing exactly that justification work, so I'm treating it as data, not as reassurance. Suggested resolution: invert the default so `require_human` is on unless a repo explicitly opts out, or restrict shadow-verdict-only merging to a narrow, non-privileged change class (e.g. lockfile/formatting) that cannot touch CI config, deploy scripts, or the automerge/review logic itself. **Scope note** The diff as provided contains only the header-comment change; the implementation of `automerge_require_human` and the shadow-verdict gate is not included in the hunk I was given. I reviewed this on the basis that the comment accurately describes the behavior it ships alongside — that is the file's own contract-documentation block, not incidental prose. If the corresponding code is *not* in this branch, the finding downgrades to a docs/code mismatch and should be re-reviewed against the real implementation; if the code *is* in this branch but was omitted from the diff I received, the finding stands as written and the omission itself is worth confirming. I flagged nothing on the remaining comment edits (CI-green, mergeable, not-behind, self-repo exclusion) — those are additive restrictions or accurate descriptions of existing behavior and introduce no new exposure. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.) _(Appended to the issue description above -- removing `Status/Blocked` re-queues the ticket with this context already in hand.)_
igor removed their assignment 2026-08-09 22:05:05 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
agent/498-the-review-pass-links-the-wrong-issue-when-a-pr
agent/439-event-driven-needs-you-notification-so-the
No results found.
Labels
Clear labels   
Agent

   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Agent
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
joshtronic
1 participant
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
joshtronic/igor#480
No description provided.