Skip to content

fix(review): serialize public-surface publish under the per-PR actuation lock (#9013) - #9181

Merged
JSONbored merged 1 commit into
mainfrom
fix/9013-pr-actuation-lock-spans-publish
Jul 27, 2026
Merged

fix(review): serialize public-surface publish under the per-PR actuation lock (#9013)#9181
JSONbored merged 1 commit into
mainfrom
fix/9013-pr-actuation-lock-spans-publish

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • maybePublishPrPublicSurface ran with no per-PR mutex; only the later maybeRunAgentMaintenance claimed the actuation lock. Two concurrent passes for the same PR (a webhook delivery racing a sweep re-review) could both publish — producing duplicate gate check-runs and letting a lock-losing pass's placeholder verdict overwrite a real one, whichever PATCHed last.
  • Claims the actuation lock once, before the publish call, at both call sites (reReviewStoredPullRequest, handlePullRequestWebhookEvent), and threads it through as preAcquiredActuationLock into both maybePublishPrPublicSurface (which also covers its internal type-label section, the only other place inside publish that claims this same lock) and maybeRunAgentMaintenance — mirroring the existing preAcquiredAiReviewLock contract. A lock-losing pass now defers the whole publish-and-maintain unit (throws, retried by the queue) instead of racing ahead.
  • Promotes the type-label block's own lock contention from "skip the label and keep publishing anyway" to deferring the whole pass, matching the sibling agent-maintenance contention behavior.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #9013).

Validation

  • git diff --check
  • npm run actionlint (not run — no workflow files changed)
  • npm run typecheck
  • npm run test:coverage locally (attempted; the full unsharded run in this environment did not finish producing a report within a practical time budget, so coverage is instead verified via targeted tests covering every new branch: the pre-acquired vs. self-claimed lock path on both maybeRunAgentMaintenance and the type-label block, and the contention-throw path for both the new outer lock and the promoted type-label lock — see the 5 updated test files below)
  • npm run test:workers (not run — no Workers-pool-specific code touched)
  • npm run build:mcp (not run — no MCP package touched)
  • npm run test:mcp-pack (not run — no MCP package touched)
  • npm run ui:openapi:check (not run — no API/schema changes)
  • npm run ui:lint / ui:typecheck / ui:build (not run — no apps/loopover-ui changes)
  • npm audit --audit-level=moderate (pre-existing transitive brace-expansion/eslint findings, unrelated to this change, unchanged by it)
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — test/unit/queue.test.ts, test/unit/queue-2.test.ts, test/unit/queue-3.test.ts, test/unit/queue-4.test.ts, test/unit/queue-5.test.ts updated/added
  • Ran the full test/unit suite (unsharded) and confirmed every failure is pre-existing on main (verified via git stash A/B comparison): test/unit/backfill.test.ts, test/unit/queue-4.test.ts ("debounces noisy PR events"), test/unit/setup-wizard-docs-parity.test.ts, test/integration/api.test.ts ("serves installation repair diagnostics") — none touch the code paths changed here

If any required check was skipped, explain why:

  • actionlint, test:workers, build:mcp/test:mcp-pack, ui:* steps were skipped because this PR only touches src/queue/processors.ts and its Node-pool unit tests — no workflow, MCP package, or UI files changed.
  • The full unsharded test:coverage run did not finish emitting its coverage report within this environment's practical time budget (v8 coverage collection over the ~22k-test suite is expensive here); coverage on the diff was instead confirmed by exercising every new branch directly with targeted tests (see below).

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session surface touched; lock-contention negative paths are covered instead.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface touched.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below with screenshots. (N/A — no visible UI changes.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A.)

Notes

  • Root cause and fix design: maybePublishPrPublicSurface (the panel comment + gate check-run publish pass) and maybeRunAgentMaintenance (the merge/close disposition pass) always ran back-to-back for a given webhook/sweep pass, but only the second one claimed the per-PR actuation lock. Two independently-triggered passes for the same PR (different coalesce keys, so never deduped against each other) could both reach maybePublishPrPublicSurface concurrently: both would GET-latest/POST-if-absent a gate check-run (no dedup exists for check-runs, unlike panel comments' deleteDuplicateMarkerComments self-heal), and whichever PATCHed last would win — including a lock-losing AI-review pass publishing an ai_review_inconclusive placeholder over a genuine verdict.
  • The fix claims ONE lock before the publish call and threads it through both the publish and maintenance calls (release happens once, in a finally spanning both), so the two are treated as a single atomic unit — a losing pass defers entirely rather than partially completing. This also transitively closes the AI-review-lock placeholder-overwrite scenario described in the issue, since two publish passes for the same PR can no longer run concurrently at all.
  • A third call site (maybeProcessPrPanelRetrigger, the manual "Re-run LoopOver review" checkbox) also calls maybePublishPrPublicSurface but was out of scope for the issue (not mentioned in its root-cause analysis) and has no trailing maintenance call to pair with; it still self-claims the lock inside the type-label block exactly as before, and is now covered by a new regression test (test/unit/queue-4.test.ts) for the promoted contention-throw behavior.
  • Not included in this PR (left as a smaller, separable follow-up if still wanted): a durably-stored gate check-run id so a second pass PATCHes the same check-run instead of a GET-latest/POST-if-absent race, and duplicate-check-run cleanup mirroring the comment dedup. The actuation-lock fix here already satisfies the issue's stated acceptance criteria (one check-run, one panel comment, no verdict overwrite) by construction, since concurrent publish passes for the same PR can no longer overlap at all.

Closes #9013

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

…ion lock (#9013)

maybePublishPrPublicSurface ran with no per-PR mutex; only the later
maybeRunAgentMaintenance claimed one. Two concurrent passes for the same
PR (a webhook delivery racing a sweep re-review) could both publish,
producing duplicate gate check-runs and letting a lock-losing pass's
placeholder verdict overwrite a real one.

Claim the actuation lock once before the publish call and thread it
through both maybePublishPrPublicSurface (as preAcquiredActuationLock,
covering its internal type-label section) and maybeRunAgentMaintenance,
mirroring the existing preAcquiredAiReviewLock contract. A lock-losing
pass now defers the whole publish-and-maintain unit instead of racing
ahead. Also promotes the type-label block's own lock contention from
"skip the label and keep publishing" to deferring the whole pass.

Closes #9013
@JSONbored
JSONbored force-pushed the fix/9013-pr-actuation-lock-spans-publish branch from c00f480 to 301ea18 Compare July 27, 2026 06:26
@JSONbored
JSONbored merged commit 1533e4d into main Jul 27, 2026
3 of 4 checks passed
@JSONbored
JSONbored deleted the fix/9013-pr-actuation-lock-spans-publish branch July 27, 2026 06:34
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
22085 5 22080 21
View the top 3 failed test(s) by shortest run time
test/unit/setup-wizard-docs-parity.test.ts > self-host GitHub App manifest <-> docs parity (#2542) > the docs page's events sentence names every buildManifest default_event, and only those
Stack Traces | 0.0144s run time
AssertionError: expected [ 'check run', 'check suite', …(5) ] to deeply equal [ 'check run', 'check suite', …(10) ]

- Expected
+ Received

  [
    "check run",
    "check suite",
-   "deployment status",
-   "issue comment",
    "issues",
    "pull request",
    "pull request review",
-   "pull request review thread",
    "push",
-   "repository",
    "status",
-   "workflow run",
  ]

 ❯ test/unit/setup-wizard-docs-parity.test.ts:65:31
test/unit/backfill.test.ts > GitHub backfill > refreshes installation health from live GitHub App metadata
Stack Traces | 0.039s run time
AssertionError: expected [ { installationId: 123, …(19) } ] to deeply equal ArrayContaining{…}

- Expected
+ Received

- ArrayContaining [
-   ObjectContaining {
+ [
+   {
      "accountLogin": "JSONbored",
+     "authMode": "local",
+     "checkedAt": "2026-07-27T06:29:24.729Z",
+     "errorSummary": undefined,
+     "eventRemediation": [
+       {
+         "action": "No change needed.",
+         "event": "issues",
+         "ok": true,
+       },
+       {
+         "action": "No change needed.",
+         "event": "issue_comment",
+         "ok": true,
+       },
+       {
+         "action": "No change needed.",
+         "event": "pull_request",
+         "ok": true,
+       },
+       {
+         "action": "Subscribe to the pull_request_review webhook event.",
+         "event": "pull_request_review",
+         "ok": false,
+       },
+       {
+         "action": "No change needed.",
+         "event": "repository",
+         "ok": true,
+       },
+       {
+         "action": "Subscribe to the check_run webhook event.",
+         "event": "check_run",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the check_suite webhook event.",
+         "event": "check_suite",
+         "ok": false,
+       },
+     ],
+     "events": [
+       "issues",
+       "issue_comment",
+       "pull_request",
+       "repository",
+       "installation_repositories",
+     ],
      "installationId": 123,
-     "missingEvents": [],
+     "installedReposCount": 0,
+     "missingEvents": [
+       "pull_request_review",
+       "check_run",
+       "check_suite",
+     ],
      "missingPermissions": [],
-     "status": "healthy",
+     "optionalPermissions": {
+       "checks": "write",
+     },
+     "optionalVisibleEvents": [
+       "installation_target",
+       "installation_repositories",
+     ],
+     "permissionRemediation": [
+       {
+         "action": "No change needed.",
+         "currentAccess": "read",
+         "ok": true,
+         "permission": "metadata",
+         "requiredAccess": "read",
+       },
+       {
+         "action": "No change needed.",
+         "currentAccess": "write",
+         "ok": true,
+         "permission": "pull_requests",
+         "requiredAccess": "read",
+       },
+       {
+         "action": "No change needed.",
+         "currentAccess": "write",
+         "ok": true,
+         "permission": "issues",
+         "requiredAccess": "write",
+       },
+     ],
+     "permissions": {
+       "checks": "write",
+       "issues": "write",
+       "metadata": "read",
+       "pull_requests": "write",
+     },
+     "registeredInstalledCount": 0,
+     "repairSteps": [
+       "Update the GitHub App permissions and subscribed events.",
+       "Approve the changed permissions or reinstall the app on the target account.",
+       "Run refresh-installation-health after GitHub sends the updated installation payload.",
+       "Recheck /v1/readiness and this installation health endpoint.",
+     ],
+     "repositorySelection": "selected",
+     "requiredEvents": [
+       "issues",
+       "issue_comment",
+       "pull_request",
+       "pull_request_review",
+       "repository",
+       "check_run",
+       "check_suite",
+     ],
+     "requiredPermissions": {
+       "issues": "write",
+       "metadata": "read",
+       "pull_requests": "read",
+     },
+     "status": "needs_attention",
    },
  ]

 ❯ test/unit/backfill.test.ts:1369:37
test/unit/backfill.test.ts > GitHub backfill > uses installation source for queued segment jobs and sparse live installation fallback metadata
Stack Traces | 0.157s run time
AssertionError: expected [ { installationId: 123, …(19) } ] to deeply equal ArrayContaining{…}

- Expected
+ Received

- ArrayContaining [
-   ObjectContaining {
+ [
+   {
      "accountLogin": "JSONbored",
+     "authMode": "local",
+     "checkedAt": "2026-07-27T06:29:30.759Z",
+     "errorSummary": undefined,
+     "eventRemediation": [
+       {
+         "action": "Subscribe to the issues webhook event.",
+         "event": "issues",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the issue_comment webhook event.",
+         "event": "issue_comment",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the pull_request webhook event.",
+         "event": "pull_request",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the pull_request_review webhook event.",
+         "event": "pull_request_review",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the repository webhook event.",
+         "event": "repository",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the check_run webhook event.",
+         "event": "check_run",
+         "ok": false,
+       },
+       {
+         "action": "Subscribe to the check_suite webhook event.",
+         "event": "check_suite",
+         "ok": false,
+       },
+     ],
      "events": [],
      "installationId": 123,
+     "installedReposCount": 1,
      "missingEvents": [
        "issues",
        "issue_comment",
        "pull_request",
+       "pull_request_review",
        "repository",
+       "check_run",
+       "check_suite",
      ],
      "missingPermissions": [
        "metadata",
        "pull_requests",
        "issues",
      ],
+     "optionalPermissions": {
+       "checks": "write",
+     },
+     "optionalVisibleEvents": [
+       "installation_target",
+       "installation_repositories",
+     ],
+     "permissionRemediation": [
+       {
+         "action": "Set repository permission metadata to read.",
+         "currentAccess": "missing",
+         "ok": false,
+         "permission": "metadata",
+         "requiredAccess": "read",
+       },
+       {
+         "action": "Set repository permission pull_requests to read.",
+         "currentAccess": "missing",
+         "ok": false,
+         "permission": "pull_requests",
+         "requiredAccess": "read",
+       },
+       {
+         "action": "Set repository permission issues to write.",
+         "currentAccess": "missing",
+         "ok": false,
+         "permission": "issues",
+         "requiredAccess": "write",
+       },
+     ],
      "permissions": {},
+     "registeredInstalledCount": 1,
+     "repairSteps": [
+       "Update the GitHub App permissions and subscribed events.",
+       "Approve the changed permissions or reinstall the app on the target account.",
+       "Run refresh-installation-health after GitHub sends the updated installation payload.",
+       "Recheck /v1/readiness and this installation health endpoint.",
+     ],
      "repositorySelection": "selected",
+     "requiredEvents": [
+       "issues",
+       "issue_comment",
+       "pull_request",
+       "pull_request_review",
+       "repository",
+       "check_run",
+       "check_suite",
+     ],
+     "requiredPermissions": {
+       "issues": "write",
+       "metadata": "read",
+       "pull_requests": "read",
+     },
      "status": "needs_attention",
    },
  ]

 ❯ test/unit/backfill.test.ts:4724:34
test/unit/backfill.test.ts > GitHub backfill > reports installation health from stored permissions and events
Stack Traces | 0.19s run time
AssertionError: expected { installationId: 123, …(19) } to match object { status: 'needs_attention', …(3) }
(22 matching properties omitted from actual)

- Expected
+ Received

@@ -1,10 +1,13 @@
  {
    "missingEvents": [
      "issues",
      "issue_comment",
+     "pull_request_review",
      "repository",
+     "check_run",
+     "check_suite",
    ],
    "missingPermissions": [
      "pull_requests",
      "issues",
    ],

 ❯ test/unit/backfill.test.ts:686:37
test/integration/api.test.ts > api routes > serves installation repair diagnostics and refreshes installation health
Stack Traces | 0.23s run time
AssertionError: expected { …(12) } to match object { refreshed: true, …(2) }
(37 matching properties omitted from actual)

- Expected
+ Received

@@ -1,10 +1,14 @@
  {
    "installation": {
-     "missingEvents": [],
+     "missingEvents": [
+       "pull_request_review",
+       "check_run",
+       "check_suite",
+     ],
      "missingPermissions": [],
-     "status": "healthy",
+     "status": "needs_attention",
    },
    "refreshed": true,
    "requiredPermissions": {
      "checks": "write",
      "issues": "write",

 ❯ test/integration/api.test.ts:2364:35

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orb(review): public-surface publish runs outside the per-PR mutex — duplicate gate check-runs and placeholder overwriting a real verdict

1 participant