Skip to content

fix(agent-actions): re-check the linked-issue hard rule on merge accept - #2388

Merged
JSONbored merged 3 commits into
mainfrom
claude/approval-queue-linked-issue-recheck
Jul 1, 2026
Merged

fix(agent-actions): re-check the linked-issue hard rule on merge accept#2388
JSONbored merged 3 commits into
mainfrom
claude/approval-queue-linked-issue-recheck

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What

decidePendingAgentAction (the approval-queue accept handler for a staged auto_with_approval action) re-validated only the head SHA before replaying a staged merge. The linked-issue hard rule (owner-assigned / missing-point-label / maintainer-only) is evaluated fresh on every planning pass and takes precedence over merge (planAgentMaintenanceActions never plans a merge when the rule is violated) — but a staged merge only replays the PLAN-TIME snapshot. If a maintainer relabels or reassigns the linked issue between staging and accept (head SHA unchanged, so the existing freshness check doesn't catch it), the stale merge decision would still execute, merging a PR that would now trip the deterministic hard-rule close if the planner were re-run.

Fix

decidePendingAgentAction (src/services/agent-approval-queue.ts) now re-runs resolveLinkedIssueHardRule for a staged merge action before executing it, denying/superseding the same way the existing head-moved check already does (same audit event type, agent.pending_action.superseded, outcome: "denied").

The recheck mirrors the planner's own closeEligible exemption: it's skipped entirely for an owner- or automation-authored PR (unless closeOwnerAuthors is on), since the hard rule never blocks those PRs in the planner either — without this, an owner's legitimate staged merge could be wrongly denied just because their own linked issue happens to match a rule pattern that was never meant to constrain them.

Scope note on the sibling CI-recheck deliverable: issue #2132 also asks to re-check live CI state at accept time. That's already covered by a separate, already-open PR for #2128 (executeAgentMaintenanceActions's step 6 CI re-verification), which applies to every merge-class action regardless of caller — decidePendingAgentAction's existing call into that executor will pick it up automatically once that PR merges, with zero additional changes needed here. This PR is deliberately scoped to just the linked-issue portion to stay small and avoid duplicating code already in flight; opening as "Advances" rather than "Closes" since full closure depends on that sibling PR landing too.

Tests

  • New test: a staged merge is superseded when resolveLinkedIssueHardRule reports a violation between staging and accept (contributor PR).
  • New test: a staged merge executes normally when the linked issue remains eligible.
  • New test: an owner-authored staged merge executes normally even when the hard-rule resolver would report a violation, and confirms the resolver is never even called (the closeEligible exemption).
  • New test: the supersede audit detail falls back to a generic reason when the hard-rule result omits one.
  • New test: a slash-less repoFullName and a PR with no author login are tolerated (the two defensive fallbacks in the new repoOwner/authorLogin derivation).
  • All 18 pre-existing tests in the file continue to pass unchanged, confirming non-merge action classes and the existing head-moved check are unaffected.
  • npx tsc --noEmit clean.
  • Scoped: agent-approval-queue.test.ts — 23 passed.
  • Regression sweep: routes-agent-approval.test.ts, mcp-automation-state.test.ts, agent-action-executor.test.ts, agent-actions.test.ts, linked-issue-hard-rules.test.ts (the two direct importers of this module plus the hard-rule evaluator itself) — 215 passed.
  • Diff-range coverage-gap check on src/services/agent-approval-queue.ts: fully covered.
  • Full unsharded npm run test:coverage: 5605 passed, 4 skipped (pre-existing/unrelated), 0 failed.
  • npm audit --audit-level=moderate: 0 vulnerabilities.

Advances #1936. Advances #2132.

@dosubot dosubot Bot added the size:M 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:25:03 UTC

2 files · 1 AI reviewer · no blockers · readiness 68/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The accept path now revalidates staged merge actions against the live linked-issue hard-rule state after precision-breaker downgrades, which is the right layer for preventing stale queued merges from bypassing deterministic planning rules. The visible implementation preserves the existing head/CI freshness checks, mirrors the planner's owner/automation exemption, and rejects only when the post-downgrade plan still contains a merge. The tests cover the main stale-rule path, the downgraded-hold path, fail-open token minting, owner exemption, and fallback audit detail.

Nits — 7 non-blocking
  • nit: test/unit/agent-approval-queue.test.ts:368 queues a mocked hard-rule violation in a test that asserts the resolver is not called, so confirm the file's beforeEach resets mock implementations as well as call counts to avoid leaking that once-value into the next test.
  • nit: src/services/agent-approval-queue.ts:149 adds a very long explanatory comment block in an already dense function; consider moving the planner-parity rationale into a small helper name or a shorter comment so the accept flow stays readable.
  • nit: src/services/agent-approval-queue.ts:157 manually derives repoOwner from repoFullName; if the repo already has a shared parser/helper for full names, use that here to avoid a second subtly different owner extraction convention.
  • In test/unit/agent-approval-queue.test.ts:368, remove the unused mockResolvedValueOnce from the no-call test or explicitly reset it before the next test so the assertion stays local to that case.
  • In src/services/agent-approval-queue.ts:157, extract the linked-issue accept-time recheck into a private helper that takes the pending row, PR, settings, and plan; that would make the closeEligible parity easier to compare against the planner.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
  • 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 ⚠️ Missing No linked issue or no-issue rationale found.
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; no linked issue context).
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.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • 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 (6556a0f) to head (9fa37e7).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2388   +/-   ##
=======================================
  Coverage   95.88%   95.88%           
=======================================
  Files         224      224           
  Lines       25143    25158   +15     
  Branches     9143     9150    +7     
=======================================
+ Hits        24109    24124   +15     
  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 added 2 commits July 1, 2026 16:07
decidePendingAgentAction re-validated only the head SHA before
replaying a staged auto_with_approval merge. The linked-issue hard
rule (owner-assigned / missing-point-label / maintainer-only) is
evaluated fresh on every planning pass and takes precedence over
merge, but a staged merge only replayed the plan-time snapshot: a
maintainer relabeling or reassigning the linked issue between staging
and accept (head SHA unchanged) would still merge a now-ineligible PR.

Re-run resolveLinkedIssueHardRule for a staged merge before executing
it, superseding the same way the head-moved check already does. Skips
the check for an owner/automation-authored PR (unless
closeOwnerAuthors is on), mirroring the planner's own closeEligible
exemption so a trusted PR the rule never blocks in the first place
isn't wrongly denied here.

Live CI re-verification for this same accept path is covered
separately by the already-open PR for #2128, which
executeAgentMaintenanceActions applies to every merge regardless of
caller — no changes needed here for that part.
…ure path

The rebase onto main's later head-pinning fixes shifted this diff's
covered range; close the resulting branch-coverage gap on the
createInstallationToken(...).catch(() => undefined) fallback added for
the #2132 linked-issue hard-rule recheck.
@JSONbored
JSONbored force-pushed the claude/approval-queue-linked-issue-recheck branch from 3ea9235 to d357bb8 Compare July 1, 2026 23:10
Comment thread src/services/agent-approval-queue.ts
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jul 1, 2026
…de plan

The linked-issue hard-rule recheck gated on pending.actionClass (the
ORIGINAL staged class) rather than the plan's actual contents after the
#2127 precision-breaker downgrade. A merge already downgraded to a
needs-human-review label by downgradeMergeToHold would still get its
whole row rejected on a stale linked-issue violation, silently swallowing
the hold label the breaker was supposed to guarantee -- since nothing is
about to merge, the linked-issue state is irrelevant to what plan is
actually going to execute.

Also documents why the recheck's best-effort token mint is intentionally
fail-open, consistent with the sibling #2126 CI/mergeable/review
re-check: resolveLinkedIssueHardRule already degrades to
env.GITHUB_PUBLIC_TOKEN before ever returning "not violated," and this is
the same shared resolver + fail-open contract the live planning path
already relies on for the primary hard-rule decision.
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jul 1, 2026
@JSONbored
JSONbored merged commit 22ec2e4 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 claude/approval-queue-linked-issue-recheck branch July 1, 2026 23:28
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.

1 participant