Skip to content

fix(review): enforce close autonomy on draft-dodge and reopen-reclose paths - #4637

Merged
JSONbored merged 1 commit into
mainfrom
fix/close-enforcement-authorization-gap
Jul 10, 2026
Merged

fix(review): enforce close autonomy on draft-dodge and reopen-reclose paths#4637
JSONbored merged 1 commit into
mainfrom
fix/close-enforcement-authorization-gap

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What

closeDraftDodgeAttemptIfBlocked and recloseDisallowedReopenIfNeeded are 2 of the 5 deterministic
close-enforcement paths that bypass the unified planAgentMaintenanceActions planner by design. Unlike
their 3 correct siblings (closeReviewEvasionSelfCloseIfActive/DraftConversionIfActive/
RepeatedDraftCyclingIfDetected), neither checked the close-specific autonomy class before closing:

  • closeDraftDodgeAttemptIfBlocked called resolveAgentPermissionReadiness(...) without
    actionClass: "close", so it checked the union of any acting autonomy class's write-permission
    grant instead of specifically close's.
  • recloseDisallowedReopenIfNeeded gated only on isAgentConfigured(...) (true when any class is
    acting), never on resolveAutonomy(..., "close").

Impact: a repo that enables any other autonomy class (e.g. assign: "auto") but deliberately leaves
close unconfigured (documented deny-by-default) could still have PRs auto-closed via these 2 paths.

Fix

Both now resolve close autonomy directly and deny (with an audit event, matching the 3 correct
siblings' shape) when it is not "auto".

Scope note

This issue's second acceptance criterion (extract a shared enforceDeterministicClose helper across all
5 close paths, ~600-700 line reduction) is deferred to an immediate fast-follow PR — keeping this fix
minimal and independently reviewable given the security-sensitive nature of the change.

Testing

4 new regression tests (draft-dodge × {denied, auto_with_approval}, reopen-reclose × {denied,
auto_with_approval}) asserting no write call and a denied audit outcome when close autonomy isn't
auto. Verified these fail pre-fix and pass post-fix. Typecheck clean; full queue.test.ts suite
(787 tests) passes.

Fixes #4602

… paths

closeDraftDodgeAttemptIfBlocked and recloseDisallowedReopenIfNeeded bypass
the unified maintenance planner by design (procedural-integrity enforcement,
not a merit verdict), but unlike their 3 correct siblings
(closeReviewEvasionSelfCloseIfActive/DraftConversionIfActive/
RepeatedDraftCyclingIfDetected) they never checked the close-specific
autonomy class -- draft-dodge checked no actionClass at all (so any acting
autonomy class satisfied it), and reopen-reclose only checked
isAgentConfigured (true for any acting class). A repo that opts into some
other autonomy class while deliberately leaving close unconfigured
(deny-by-default) could still get PRs auto-closed via either path.

Both now resolve the close autonomy class directly and deny (with an audit
event) when it is not "auto", mirroring the 3 already-correct siblings.

Extraction of the shared enforceDeterministicClose helper across all 5 close
paths (this issue's second acceptance criterion) is deferred to an immediate
fast-follow PR to keep this security fix minimal and reviewable in
isolation.

Fixes #4602
@superagent-security

Copy link
Copy Markdown
Contributor

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

@JSONbored JSONbored self-assigned this Jul 10, 2026
@JSONbored
JSONbored merged commit 66e4943 into main Jul 10, 2026
7 checks passed
@JSONbored
JSONbored deleted the fix/close-enforcement-authorization-gap branch July 10, 2026 08:43
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.10%. Comparing base (19e1654) to head (1ab582f).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 83.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4637      +/-   ##
==========================================
- Coverage   94.11%   94.10%   -0.01%     
==========================================
  Files         432      432              
  Lines       38371    38382      +11     
  Branches    13989    13994       +5     
==========================================
+ Hits        36111    36120       +9     
  Misses       1602     1602              
- Partials      658      660       +2     
Files with missing lines Coverage Δ
src/queue/processors.ts 95.25% <83.33%> (-0.05%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added a commit that referenced this pull request Jul 10, 2026
Part of #4602 (second acceptance criterion): the 5 deterministic close-
enforcement paths in src/queue/processors.ts each duplicated ~150-230 lines
of scaffolding -- agent-action-mode resolution, the close-autonomy check,
dry-run/paused stand-down, write-permission readiness, and the live
freshness re-check -- the exact class of duplication that let 2 of the 5
silently miss the close-autonomy check (fixed in #4637).

Extracts withPrActuationLock (the byte-identical lock claim/try/finally-
release wrapper) and evaluateCloseEnforcementGate (the mode/autonomy/
standdown/permission-readiness/freshness scaffolding, parameterized by each
caller's exact detail text and metadata) so all 5 call sites now share one
function for that scaffolding. The actual GitHub mutation and its post-
success side effects stay caller-owned, since they differ too much to unify
(self-close's reopen-then-close with asymmetric error handling vs. the
other 4's single close call).

Two pre-existing, intentionally-preserved asymmetries: draft-dodge records
no audit event on a paused/frozen repo (every other guard does), and
reopen-reclose has never had a write-permission-readiness check (every
other guard does). Neither is introduced or fixed here -- a refactor must
not change behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Fix close-enforcement authorization gap; extract shared helper

1 participant