Repository navigation
π fix: evict locally merged PRs from tracker snapshots - #14424
thatguywob wants to merge 4 commits into
Conversation
Apply terminal PR attribution audits through the shared snapshot patch path so the tracker and governor stop listing them before webhooks arrive. Hive-Run: hivecommons#14420 Hive-Plan: local-terminal-pr-snapshot-eviction Hive-Spec: hivecommons#14420#fix Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com> Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
@thatguywob this PR is back in your court ( Failing checks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity. |
There was a problem hiding this comment.
Looks correct to me.
What I verified against the tree at this head:
- The record shape matches what
ApplyPRTerminalAuditreads. Both emitters go throughrecordWriteAuditβwriteAuditRecord(src/pkg/github/write_surface.go), which setsrec.RepofromWriteTarget.Repoandrec.TargetfromWriteTarget.Number;RecordPRMergedAudit(client.go:4350) and the relay close path (issue_request_watcher.go:845-851) both pass the PR number there. The bare-name qualification is harmless either way sinceapplyWebhookItemSnapshotsalready matches on bothoverviewRepoNameandrepo.Name(webhook_tracker_refresh.go:341). - The eviction and in-flight replay are real, not new code:
applyWebhookItemSnapshots(webhook_tracker_refresh.go:327) forwards todeps.ApplyWebhookItems, callsrememberWebhookPatchesLockedbefore patching, copiesRepos(copy-on-write) and invalidates the tracker. A closed snapshot skips thesnapshotOlderThanListedguard, which is the right behaviour for a terminal event the hive itself just performed. This is the same path the webhook handler uses at:288, so the new producer adds no second code path to keep in sync. - No re-entrancy risk:
deliverAuditRecordreleasesattribMubefore invoking the callback, and the existing callback body ingithub_client_config.go:328-332already calls intodashSrv, so the lock context is unchanged.
The red docker (contributor) check does not touch anything in this diff (the PR changes no Dockerfile or image inputs); it looks like the same contributor-image breakage #14383 addresses.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
The contributor Docker build received HTTP 404 for bowser-2.15.0.tgz during the Pi install. The same registry URL now returns HTTP 200. Trigger a fresh CI run without changing the dependency pins or PR implementation. Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
|
@thatguywob this PR is back in your court ( Failing checks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity. |
There was a problem hiding this comment.
Core change looks correct to me; one scope note for the body.
Verified (read at head c93a583):
- Both emission sites fire only after a successful write:
RecordPRMergedAudit(src/pkg/github/client.go:4350-4358, every merge path) and the relay'spr_closedbranch (src/pkg/github/issue_request_watcher.go:845-850, after the close response). SoApplyPRTerminalAuditnever evicts a PR the hive failed to merge/close. applyWebhookItemSnapshots(src/pkg/dashboard/webhook_tracker_refresh.go:327-367) is the same path webhooks use for closed PRs: it patches the governor's cached snapshot viasnapshotPatcher.apply(src/cmd/hive/eval_cycle_source.go:90-120, own mutex + atomic clone, no shared lock with the merge sweep), thenrememberWebhookPatchesLockedbefore patchings.status, so an in-flight publish re-applies the close. No deadlock path from the audit callback;deliverAuditRecord(attribution.go:581-596) only holdsattribMu.RLockand releases it before invoking the hook.- Hook wired next to the existing
recordLifecycleFromAuditingithub_client_config.go:331, so it runs for every client config site.
Scope / body mismatch (low): commit c93a583 adds a 3-attempt retry loop to the Pi install in src/Dockerfile.contributor:488-497. That is unrelated to #14420, is not mentioned in the PR body or changelog fragment, and lands in a PR titled "evict locally merged PRs". The loop itself is fine (exit 1 on the third failure still fails the build; done && npm cache clean is valid sh). Suggest either mentioning it in the body so the squash message is honest, or moving it to its own π± PR β a maintainer bisecting a contributor-image change later will not look here.
Open question, not a finding: the rollup shows test and coverage red alongside test (agent 2/5); agent 2/5 is the known base-branch failure (#14406) also red on #14395, but I did not confirm whether coverage is the same incident or the dashboard package floor. Worth a glance before merge.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
c93a583
|
|
@thatguywob the hive has adopted this PR ( |
The Dockerfile.contributor retry around the Pi npm install was added to unblock CI but was not described in the PR body or changelog; record it so the squash message explains the change. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
@thatguywob the hive has adopted this PR ( |
There was a problem hiding this comment.
This PR is now byte-identical to #14462: both sit at head 7c0c43a, same 5 files (pr_terminal_audit.go, its test, github_client_config.go, Dockerfile.contributor, changelog fragment). I read both diffs in full; there is no content difference.
#14462 is the hive's App-authored re-home of this branch so the squash sign-off attributes correctly (per its body). I posted the code review there β the eviction logic looks correct to me, reusing applyWebhookItemSnapshots at src/pkg/dashboard/webhook_tracker_refresh.go:327.
Flagging for a maintainer: if you agree, this one could be closed in favour of #14462. I have not closed anything.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Closing: #14462 (the hive's adoption of this PR, with @thatguywob's commits) has merged. Thank you! |
Fixes #14420
Successful
pr_mergedandpr_closedattribution audits now immediately apply a closed PR snapshot through the existing webhook patch path. This removes the PR from the governor's cached actionable snapshot and published tracker, and remembers the patch so an in-flight enumeration cannot restore the row. Bare repository names are qualified with the configured organization; no GitHub read is needed.Added regression coverage for merge/close eviction, actionable and sidelined lists, governor patch forwarding, stale status replay, copy-on-write behavior, and invalid/non-terminal records. Added a changelog fragment.
Also hardens
src/Dockerfile.contributor: the pinned Pi coding-agent npm install is retried up to three times with a 30s delay, because registry tarballs can briefly 404 while a release propagates and npm does not retry 404s. This was needed to get CI green for this PR and is recorded in the changelog fragment.Validation:
gofmt -landgit diff --checkpass. Tests are reserved for CI per AGENTS.md and were not run locally.β hive: backend=codex model=gpt-6.1-sol
π Hive Agent:
contributor| SHA:b8e7b3f0d