fix: reconcile a dead pin against declared evidence, not a branch guess (SYD-273) - #237
Merged
Merged
Conversation
…ss (SYD-273) Two changes, both aimed at the same 15-day loop on SYD-108: 8 redeliver_requested events, each answered by a byte-identical delivery_failed, two of them 25 minutes apart. 1. The closed-pin reconcile asks the TRACKER before GitHub. SYD-232 added a check for a replacement PR, but scoped it to `gh pr list --head agent/<ref>`. That asks where work lives rather than what delivered it, and it is wrong for exactly the case that produced this bug: SYD-108's #61 closed unmerged and its replacement #124 merged from feat/syd-108-gate-delivery-events, so every retry reported "no later merged PR" about work that had been on main for 15 days. The authorization now carries deliveredByPrNumber -- the issue's proof-bearing declared link joined to a merged pr_state row, on whatever branch. Deliberately NOT sourced from gh_pr_merged events, which the issue proposed: this value makes the worker record `delivered` and mark the attempt merged_deployed, so it has to meet the SYD-280 bar of a declared link rather than a ref someone typed into a PR title. pr_state has no merge-sha column, so GitHub still supplies the SHA -- a targeted lookup of one known PR, not a search. The branch query remains as the fallback it was written for: a re-dispatch reusing agent/<ref>. 2. redeliverIssue refuses a pin that already dead-ended. The poll is a pure function of the pin, so an identical pin can only produce an identical answer. Gated on the pin being CLOSED so the guard can't become a wall -- reopening the PR is the case where retrying the same pin is right, and pr_state knows when that happened. A moved head is a different pin and was never the doomed one. The refusal names the exits: declare the PR that actually carries the work (the tracker then reconciles on its own), or resolve-delivery with a note. Both failure messages now say the declared-link path was checked, so "no PR on the branch" can't be misread as "we only looked at the branch". Repo comparison is case-folded, matching the pr_state readers -- SYD-108 records the merge under MobilityLabs/switchyard and the pins under mobilitylabs/switchyard, the SYD-212 hazard live in the data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes aimed at the same loop: SYD-108 accumulated 8
redeliver_requestedevents over 15 days, each answered by a byte-identicaldelivery_failed, two of them 25 minutes apart.1. The closed-pin reconcile asks the tracker before GitHub
SYD-232 added a replacement-PR check but scoped it to
gh pr list --head agent/<ref>. That asks where work lives rather than what delivered it, and it's wrong for precisely the case that produced this bug:#61—agent/SYD-108, closed unmerged (the pin)#124—feat/syd-108-gate-delivery-events, mergedaea0dd671377, titled "fix: gate delivery-events against agent actors (SYD-108)"The branch query can never see #124, so every retry reported "no later merged PR" about work that had been on
mainfor 15 days.The authorization now carries
deliveredByPrNumber: the issue's proof-bearing declared link joined to a mergedpr_staterow, on whatever branch.Deliberately not sourced from
gh_pr_mergedevents, which is what the issue proposed. This value makes the worker recorddeliveredand mark the attemptmerged_deployed— it declares the work shipped. That has to meet the SYD-280 evidence bar (a declared link) rather than the SYD-280-rejected one (a ref someone typed into a PR title).attention.tsputs it plainly: "clearing a delivery failure re-authorizes an actual merge+deploy, so the evidence has to be a declared link joined to an observed merge."pr_statehas no merge-SHA column, so GitHub still supplies the SHA — but as a targeted lookup of one known PR, not a search. The branch query stays as the fallback it was written for: a re-dispatch reusingagent/<ref>.2.
redeliverIssuerefuses a pin that already dead-endedThe poll is a pure function of the pin, so an identical pin can only produce an identical answer. Re-running it costs a GitHub round-trip and re-asserts "work never landed" on an issue whose work is on
main.Gated on the pin being CLOSED, so the guard can't become a wall — reopening the PR is exactly the case where retrying the same pin is right, and
pr_stateknows when that happened. A moved head is a different pin and was never the doomed one. Both are covered by tests that assert the retry still goes through.The refusal names the exits rather than just saying no: declare the PR that actually carries the work (the tracker then reconciles on its own), or
resolve-deliverywith a note.Worth flagging: this does not fix SYD-108 by itself
I checked before building, and SYD-108's links are
#61 delivers(the closed one) and#186 references. PR #124 is not linked at all, sogetMergedPrreturns null anddeliveredByPrNumberwill be null for it.That's the real root cause, and it's upstream of this check: SYD-108's work landed through an interactive
feat/branch and nobody declared the link — the gap SYD-280 exists to close. Declaring #124 as SYD-108'sdeliverslink is what unblocks it; this PR is what stops the next one happening. Both failure messages now say the declared-link path was checked, so "no PR on the branch" can't be misread as "we only looked at the branch".Also
Repo comparison in the new guard is case-folded, matching the
pr_statereaders — suggestion 2 in the issue. Worth noting it's live in the data: SYD-108 records the merge underMobilityLabs/switchyardand its pins undermobilitylabs/switchyard.Verification
npm test2044 passed / 167 files ·npm run typecheckclean · eslint + prettier clean.Two existing tests needed updating, both mine to break: the
/delivery-workexact-shape assertion (additive field) and the dead-end comment wording. I tightened both rather than loosening them — the deliver test now also asserts the message mentions the declared-link path and the declare-the-real-PR exit.