Skip to content

fix(agent-actions): re-verify a staged merge live state and precision breaker at accept time - #2354

Merged
JSONbored merged 5 commits into
mainfrom
claude/approval-queue-live-recheck
Jul 1, 2026
Merged

fix(agent-actions): re-verify a staged merge live state and precision breaker at accept time#2354
JSONbored merged 5 commits into
mainfrom
claude/approval-queue-live-recheck

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What

decidePendingAgentAction's only freshness check before replaying a staged approval-queue action was head-SHA equality. auto_with_approval rows have no expiry, so between staging and a maintainer's accept: CI could flip red, the base could go dirty, a reviewer could request changes, or the merge-precision circuit-breaker could engage — none of which move the head SHA, so none of them were caught.

Fixes

Bonus fix found while wiring the close breaker

closeKind never survived staging at all — actionParams() silently dropped it when persisting a planned action's params, so downgradeCloseToHold's closeKind === "heuristic" match could never fire for any staged close, regardless of this fix. Threaded closeKind through AgentPendingActionParams and actionParams() so it round-trips from staging to accept.

Tests

  • Three staleness-supersede cases (CI failed / mergeable dirty / changes-requested), each asserting the merge is denied, not executed.
  • Merge-method re-sync to current config.
  • Precision breaker downgrades a staged merge to a label, and a staged heuristic close to a label; a clean accept with the breaker off still executes normally.
  • Fail-open regression: the live re-check's own token-mint failure doesn't block the accept (the executor's own mutation call handles that independently).
  • actionParams round-trips closeKind.

Full unsharded test:coverage green; typecheck green.

Advances #1936. Closes #2126, #2127, #2131. Advances #2132.

… at accept time

decidePendingAgentAction's only freshness check before replaying a staged
approval-queue action was head-SHA equality. auto_with_approval rows have no
expiry, so between staging and a maintainer's accept, CI could flip red, the
base could go dirty, a reviewer could request changes, or the merge-precision
circuit-breaker could engage — none of which move the head SHA, so none of
them were caught.

- Re-fetch live CI state, mergeable_state, and reviewDecision for a staged
  merge at accept time; supersede (deny, audit, leave the row untouched)
  instead of executing on stale justification. Best-effort: a failed live
  read fails open on that specific check, since the mutation call
  independently needs a valid token/state and fails cleanly on its own.
- Re-apply the same merge/close precision circuit-breakers the live webhook
  path already applies, so a breaker engaged after staging still holds the
  row (downgrades to a needs-human-review label) instead of executing
  unmodified.
- Re-sync the merge method to the repo's current config instead of the
  staging-time snapshot.

While wiring the close breaker, found `closeKind` never survived staging at
all — `actionParams()` dropped it, so `downgradeCloseToHold`'s heuristic-close
match could never fire for any staged close regardless of this fix. Threaded
it through `AgentPendingActionParams` and `actionParams()` so it round-trips.

Advances #1936. Closes #2126, #2127, #2131. Advances #2132 (the CI/mergeable/
review portion lands here; the linked-issue-hard-rule re-check does not — see
that issue for the remaining scope).
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.72%. Comparing base (17d70b2) to head (5c6125d).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2354   +/-   ##
=======================================
  Coverage   95.71%   95.72%           
=======================================
  Files         222      222           
  Lines       24661    24682   +21     
  Branches     8949     8961   +12     
=======================================
+ Hits        23605    23627   +22     
  Misses        433      433           
+ Partials      623      622    -1     
Files with missing lines Coverage Δ
src/services/agent-action-executor.ts 86.31% <100.00%> (+0.14%) ⬆️
src/services/agent-approval-queue.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb

loopover-orb Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-01 19:03:32 UTC

6 files · 1 AI reviewer · no blockers · readiness 75/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly moves staged merge acceptance closer to the live webhook path by rechecking CI, mergeability, review state, current merge method, and precision breakers before execution. The main implementation is coherent, and the closeKind round-trip fixes a real staging bug. One accept-time fail-open path is not actually fail-open: the code catches token creation failure but still calls the live recheck with an undefined token, and the added route test comment documents that this can fulfill as an unverified CI state and deny the action before the executor gets its own chance.

Blockers

  • src/services/agent-approval-queue.ts:75 catches createInstallationToken failure but still runs fetchLiveCiAggregate with an undefined token, so a token mint failure can produce ciState "unverified" and reject the staged merge at src/services/agent-approval-queue.ts:91 instead of failing open as documented.
Nits — 6 non-blocking
  • nit: src/services/agent-approval-queue.ts:79 should skip the three live recheck calls entirely when token minting fails if fail-open is the intended behavior, rather than relying on each fetch helper's undefined-token semantics.
  • nit: test/unit/agent-approval-queue.test.ts should cover the real token-failure shape by asserting the live fetch helpers are not called when createInstallationToken rejects, instead of mocking those helpers to return passing data.
  • src/services/agent-approval-queue.ts:75: gate the live recheck block on a successfully minted token, or explicitly make undefined-token results non-authoritative before computing staleReason.
  • test/unit/agent-approval-queue.test.ts: add an assertion in the token-mint-failure case that fetchLiveCiAggregate, fetchLivePullRequestMergeState, and fetchLivePullRequestReviewDecision were not invoked.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2126
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:M; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 67 registered-repo PR(s), 57 merged, 590 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 67 PR(s), 590 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Triage stale or unlinked PRs.
  • No action.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 1, 2026
JSONbored added 2 commits July 1, 2026 05:33
The three live re-checks (CI aggregate, mergeable state, review
decision) were awaited via a bare Promise.all, so a transient rejection
from any one of them threw out of decidePendingAgentAction instead of
failing open on that specific check -- exactly the design this code's
own comment describes, but Promise.all does not provide that isolation
even though each function already catches its own fetch errors
internally today (a future edit removing one of those internal catches
would silently reintroduce a crash with no test to catch it).

Switch to Promise.allSettled and treat a rejected settle as "nothing
concerning found" for that check, matching each function's own
already-established fail-open return value.
JSONbored added 2 commits July 1, 2026 11:43
… just failed

An accept-time live CI recheck that fulfills with "pending" or "unverified"
(rather than rejecting) previously fell through to the same non-blocking
path as "passed", letting a staged merge execute on live CI that had moved
off green without ever going red. Distinguish a genuinely non-passing
FULFILLED read from a REJECTED one (fail-open, unchanged) instead of
collapsing both into a single sentinel string.
…outes happy path

routes-agent-approval.test.ts's accept happy path never mocked
fetchLiveCiAggregate, so an unconfigured GITHUB_APP_PRIVATE_KEY left the
token undefined and the live read fulfilled with ciState "unverified" -
now a genuine stale signal instead of an accidentally-tolerated one.
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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

fix(agent-actions): approval-queue accept replays a staged merge with no live CI/mergeable/review re-check

1 participant