Summary
The scheduled sweep added in #416 is narrowed to pull requests whose head lies
outside the base repository, because those are the only ones the approval gate
can block. That removes the serious case — re-running the submission pipeline
for every open pull request on every interval — but leaves a smaller one.
A fork pull request that has already been approved and already transferred
is still visited by each sweep, and its pipeline runs again. ALLOW_DUPLICATES
defaults to true, under which a detected duplicate is logged and allowed
through, so nothing downstream stops the re-submission.
Why it is tolerable for now, and why it should not stay that way
Repeated runs are the tool's normal mode: every synchronize re-runs the same
pipeline, and reconciliation exists to make that idempotent. The affected
population is also small — fork pull requests only, and only between approval
and merge.
But the sweep is unattended and perpetual, which is exactly the property that
made the unnarrowed version dangerous. Relying on reconciliation to absorb an
hourly repeat is relying on a mechanism built for a different frequency.
Proposed approach
The gate already knows the answer. _check_fork_approval returns the commit it
authorised, and _clear_fork_approval_notice edits the notice comment to record
the approval, including a short SHA in the rendered text.
Making that machine-readable would give a natural idempotency key: extend the
marker to carry the transferred head SHA, for example
<!-- github2gerrit:fork-approval v2 transferred=<full sha> -->
A sweep could then skip a pull request whose notice already records the current
head as transferred, and process it whenever the head has moved.
Points to settle during implementation:
- The notice is written best-effort and can fail (a read-only token, for
instance). A missing marker must mean "process it", not "skip it", so a
failure to record degrades to today's behaviour rather than stalling a
transfer.
- The marker is not proof of authorship. The existing code establishes
ownership by attempting the edit (_edit_owned_marker_comment), and
anything reading the marker for control-flow purposes must not assume the
tool wrote it. Skipping is the safe direction, so a forged marker delays a
transfer rather than causing one — worth stating in the code, and worth a
test.
- Only the sweep should consult it. A dispatched run, a push and a
@github2gerrit check comment are all deliberate requests, and must keep
working regardless of what the marker says.
Acceptance criteria
Raised from review of #418.
Summary
The scheduled sweep added in #416 is narrowed to pull requests whose head lies
outside the base repository, because those are the only ones the approval gate
can block. That removes the serious case — re-running the submission pipeline
for every open pull request on every interval — but leaves a smaller one.
A fork pull request that has already been approved and already transferred
is still visited by each sweep, and its pipeline runs again.
ALLOW_DUPLICATESdefaults to
true, under which a detected duplicate is logged and allowedthrough, so nothing downstream stops the re-submission.
Why it is tolerable for now, and why it should not stay that way
Repeated runs are the tool's normal mode: every
synchronizere-runs the samepipeline, and reconciliation exists to make that idempotent. The affected
population is also small — fork pull requests only, and only between approval
and merge.
But the sweep is unattended and perpetual, which is exactly the property that
made the unnarrowed version dangerous. Relying on reconciliation to absorb an
hourly repeat is relying on a mechanism built for a different frequency.
Proposed approach
The gate already knows the answer.
_check_fork_approvalreturns the commit itauthorised, and
_clear_fork_approval_noticeedits the notice comment to recordthe approval, including a short SHA in the rendered text.
Making that machine-readable would give a natural idempotency key: extend the
marker to carry the transferred head SHA, for example
<!-- github2gerrit:fork-approval v2 transferred=<full sha> -->A sweep could then skip a pull request whose notice already records the current
head as transferred, and process it whenever the head has moved.
Points to settle during implementation:
instance). A missing marker must mean "process it", not "skip it", so a
failure to record degrades to today's behaviour rather than stalling a
transfer.
ownership by attempting the edit (
_edit_owned_marker_comment), andanything reading the marker for control-flow purposes must not assume the
tool wrote it. Skipping is the safe direction, so a forged marker delays a
transfer rather than causing one — worth stating in the code, and worth a
test.
@github2gerrit checkcomment are all deliberate requests, and must keepworking regardless of what the marker says.
Acceptance criteria
Raised from review of #418.