Skip to content

fix(agent-actions): deny an unpinned staged merge on accept - #2431

Merged
JSONbored merged 1 commit into
mainfrom
fix-2422-merge-unpinned-accept
Jul 1, 2026
Merged

fix(agent-actions): deny an unpinned staged merge on accept#2431
JSONbored merged 1 commit into
mainfrom
fix-2422-merge-unpinned-accept

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • #2377 closed the unpinned-legacy-row gap for approve. merge had the identical fallback in performAction: mergeSha = action.expectedHeadSha ?? ctx.headSha.
  • The usual "GitHub 409s on a stale sha" backstop that protects a pinned merge doesn't cover this case: the fallback substitutes whatever head is live right now, so it trivially matches and no 409 is possible. An unpinned staged merge (no expectedHeadSha, e.g. a row from before this head-pinning fix, or a planning pass that ran against a transiently-null stored head SHA) would silently merge whatever commit is live at accept time, under the authority of a review/merge that was never actually performed against it.
  • Extends the same accept-flow denial gate #2377 added for approve to also cover merge.

Closes #2422

Test plan

  • npm run typecheck clean
  • Full unsharded npm run test:coverage: 312 files / 5833 tests passing, no threshold failures (re-verified after rebasing onto #2423, which also merged into main and touches the same function)
  • npm audit --audit-level=moderate: 0 vulnerabilities
  • New regression test proves an unpinned staged merge is now denied (unpinned_legacy_action), not silently executed
  • Updated 4 pre-existing tests across 3 files that staged an unpinned merge incidentally (to exercise unrelated downstream behavior — dry-run honoring, autonomy staleness, write-permission denial, a shared route-test fixture) to pin expectedHeadSha so they keep testing their original intent instead of hitting the new gate

#2377 closed this gap for approve; merge had the identical fallback in
performAction (mergeSha = action.expectedHeadSha ?? ctx.headSha). The
usual "GitHub 409s on a stale sha" backstop doesn't cover this case: the
fallback substitutes whatever head is live right now, so it trivially
matches and no 409 is possible. Extend the same accept-flow denial gate
to merge.

Closes #2422
@dosubot dosubot Bot added the size:S label Jul 1, 2026
@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 23:01:41 UTC

4 files · 1 AI reviewer · no blockers · readiness 75/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly extends the accept-time freshness gate from unpinned approve actions to unpinned merge actions, preventing `performAction` from falling back to the current live head for a staged merge that was never pinned to a reviewed commit. The updated regression test drives the reachable accept path and verifies the merge executor is not called, while existing merge tests are pinned so they continue exercising their original downstream behavior. I do not see a reachable correctness break in the provided diff.

Nits — 4 non-blocking
  • nit: `src/services/agent-approval-queue.ts:70` now carries a long, duplicated rationale that is useful but dense; consider trimming the production comment and keeping the exhaustive scenario detail in the regression test name/body.
  • `src/services/agent-approval-queue.ts:80` could include `stagedHeadSha: null` in the audit metadata for the unpinned-denial branch, matching the stale-head branch’s explicit freshness fields and making audit queries less dependent on parsing `detail`.
  • 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 #2422
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:S; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 572 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 572 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
@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.88%. Comparing base (76e9baa) to head (51c17f0).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2431   +/-   ##
=======================================
  Coverage   95.88%   95.88%           
=======================================
  Files         224      224           
  Lines       25136    25137    +1     
  Branches     9139     9140    +1     
=======================================
+ Hits        24102    24103    +1     
  Misses        421      421           
  Partials      613      613           
Files with missing lines Coverage Δ
src/services/agent-approval-queue.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 1, 2026
@JSONbored
JSONbored merged commit 6556a0f into main Jul 1, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 1, 2026
@JSONbored
JSONbored deleted the fix-2422-merge-unpinned-accept branch July 1, 2026 23:05
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): merge has the same unpinned-legacy-row head-fallback gap as approve

1 participant