Skip to content

fix(agent-actions): add ctx.headSha fallback + rationale to update_branch - #2430

Merged
JSONbored merged 1 commit into
mainfrom
fix-2424-update-branch-parity
Jul 1, 2026
Merged

fix(agent-actions): add ctx.headSha fallback + rationale to update_branch#2430
JSONbored merged 1 commit into
mainfrom
fix-2424-update-branch-parity

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • performAction's update_branch case passed action.expectedHeadSha directly into updatePullRequestBranch with no ?? ctx.headSha fallback, unlike the approve/merge cases immediately above it — a real, if narrow, asymmetry.
  • Adds the same fallback for parity/defense-in-depth (closes a tiny window between the executor's step-5 freshness read and this call).
  • Documents why update_branch does NOT need the accept-flow-level "unpinned → deny" gate that #2377/#2422 added for approve/merge: it only merges the current base into the head (never contributor-controlled content), so it cannot itself ratify unreviewed code — the worst case is a premature rebase that fires a fresh synchronize and gets re-reviewed on the next pass. It's also already covered by the generic freshness guard and the approval-queue's actionClass-agnostic supersede check, both of which run before any action class reaches this switch.

Closes #2424

Test plan

  • npm run typecheck clean
  • Full unsharded npm run test:coverage: 312 files / 5822 tests passing, no threshold failures
  • npm audit --audit-level=moderate: 0 vulnerabilities
  • New regression test proves the ?? ctx.headSha fallback actually fires for an unpinned staged update_branch (previously untested — the existing fixture was always pre-pinned)

…anch

performAction's update_branch case passed action.expectedHeadSha directly
into updatePullRequestBranch with no ?? ctx.headSha fallback, unlike the
approve/merge cases immediately above it. Add the same fallback for
parity, and document why update_branch does NOT need the accept-flow-level
"unpinned -> deny" gate #2377/#2422 added for approve/merge: it only
merges the base into the head, never contributor-controlled content, so
it cannot itself ratify unreviewed code, and it's already covered by the
generic freshness guard + the approval-queue's actionClass-agnostic
supersede check.

Closes #2424
@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 22:49:52 UTC

2 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 brings `update_branch` into parity with the approve/merge execution path by falling back to `ctx.headSha` after the shared freshness guard has already validated the same expression. The new regression test exercises the previously unpinned action path and verifies the GitHub call receives the fallback SHA, so the visible diff is safe to proceed. The main maintainability concern is that the rationale comment in the hot switch is much larger than the behavior it guards.

Nits — 6 non-blocking
  • nit: `src/services/agent-action-executor.ts:268` carries a long policy/history explanation inside `performAction`; I would trim this to the invariant the code depends on and leave deeper rationale in the PR or issue history.
  • nit: `test/unit/agent-action-executor.test.ts:106` includes several lines of implementation rationale in the test body; the test would be easier to maintain if the comment focused only on why this fixture must be unpinned.
  • `src/services/agent-action-executor.ts:278`: keep the concise invariant near the code, for example that step 5 has already validated `action.expectedHeadSha ?? ctx.headSha`, and move the accept-flow rationale out of the switch body.
  • `test/unit/agent-action-executor.test.ts:112`: also assert that the planned action omits `expectedHeadSha` if this helper style is common in the file, so future fixture edits cannot accidentally stop covering the fallback path.
  • 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 #2424
Related work ⚠️ 2 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.85%. Comparing base (cbf1909) to head (b944b31).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2430   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files         224      224           
  Lines       25104    25104           
  Branches     9129     9130    +1     
=======================================
  Hits        24064    24064           
  Misses        427      427           
  Partials      613      613           
Files with missing lines Coverage Δ
src/services/agent-action-executor.ts 89.90% <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 merged commit 43a8a69 into main Jul 1, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix-2424-update-branch-parity branch July 1, 2026 22:56
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 1, 2026
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.

chore(agent-actions): update_branch silently omits expected_head_sha with no fallback when the pin is absent

1 participant