Skip to content

fix(queue): gate the draft-dodge close on write-permission readiness - #2391

Merged
JSONbored merged 2 commits into
mainfrom
claude/draft-dodge-permission-readiness
Jul 1, 2026
Merged

fix(queue): gate the draft-dodge close on write-permission readiness#2391
JSONbored merged 2 commits into
mainfrom
claude/draft-dodge-permission-readiness

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What

The converted_to_draft draft-dodge handler (src/queue/processors.ts) calls closePullRequest() directly instead of routing through executeAgentMaintenanceActions, since the whole point is to enforce the gate verdict against the current headSha even though the PR was converted to draft. But that means this one close path never got the standard actuation pipeline's write-permission readiness check (PR_WRITE_CLASSES / resolveAgentPermissionReadiness, step 6 in executeAgentMaintenanceActions). An installation whose pull_requests: write grant was revoked or never consented would still attempt the close, get a 403 from GitHub, and have the failure silently swallowed by the existing .catch(() => undefined) — with the github_app.draft_dodge_closed audit event still recorded with outcome: "completed" as if the close actually happened. No unauthorized mutation occurs (GitHub itself rejects it), but the audit trail becomes misleading, with no signal for an operator to notice a misconfigured/under-permissioned installation.

Fix

Added a resolveAgentPermissionReadiness({ autonomy, installationPermissions }) check at the top of the draftMode === "live" branch, before the comment/close calls — mirroring executeAgentMaintenanceActions' own step 6 exactly. installationPermissions comes from a fresh getInstallation read (this handler doesn't already hold one). When not ready, records a "denied" audit outcome (same github_app.draft_dodge_closed event type, distinguishable via outcome) instead of attempting the GitHub call at all — neither the warning comment nor the close is posted, since posting a comment that promises an imminent close would be misleading if the close itself can't happen.

Longer-term routing this path through executeAgentMaintenanceActions as a proper PlannedAgentAction (which the issue also suggests, tracked jointly with the sibling live-recheck issue) would get this guard for free — out of scope here to keep this PR small and surgical.

Tests

  • New test: the close is denied (not attempted — no PATCH, no comment POST) when the installation's granted permissions don't include pull_requests: write, with a "denied" audit outcome citing the missing grant.
  • New test: the same denial fires when no installation row was pre-synced and the webhook payload carries no permissions (processGitHubWebhook's own unconditional upsertInstallation(env, payload) auto-creates a bare record from the payload, which has no explicit write grant either).
  • All 3 pre-existing draft-dodge tests (success path, global-freeze no-op, dry-run) continue to pass unchanged.
  • npx tsc --noEmit clean.
  • Scoped: full queue.test.ts — 204 passed. agent-execution.test.ts (the module the new import comes from) — 11 passed.
  • Diff-range coverage-gap check on src/queue/processors.ts: fully covered — a couple of genuinely-defensive fallbacks (a getInstallation read failure, a null-author edge case) are marked /* v8 ignore next */, mirroring the identical established convention already used twice in this same function for the same shapes of fallback.
  • Full unsharded npm run test:coverage: 5602 passed, 4 skipped (pre-existing/unrelated), 0 failed.
  • npm audit --audit-level=moderate: 0 vulnerabilities.

Advances #1936. Closes #2134.

@dosubot dosubot Bot added the size:M label 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.89%. Comparing base (22ec2e4) to head (1a474eb).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2391   +/-   ##
=======================================
  Coverage   95.88%   95.89%           
=======================================
  Files         224      224           
  Lines       25158    25162    +4     
  Branches     9150     9151    +1     
=======================================
+ Hits        24124    24128    +4     
  Misses        421      421           
  Partials      613      613           
Files with missing lines Coverage Δ
src/queue/processors.ts 91.54% <100.00%> (+0.01%) ⬆️
🚀 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 23:43:20 UTC

2 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 adds a permission-readiness gate to the draft-dodge live close path before the freshness check, denying and auditing without posting a comment or closing when PR write permission is unavailable. That matches the standard actuation pipeline behavior for this bypass path, and the new tests cover denied permissions, missing stored permissions, and transient installation-read failure. The existing freshness guard and close/comment sequence are preserved under the ready branch.

Nits — 7 non-blocking
  • nit: src/queue/processors.ts:3779 has a very large explanatory comment that repeats the PR narrative; trimming it to the permission guard and retry rationale would keep this hot-path branch easier to scan.
  • nit: src/queue/processors.ts:3810 records every non-ready result as "pull_requests: write not granted"; include the readiness value in metadata so future non-ready states stay diagnosable.
  • nit: test/unit/queue.test.ts:11798 duplicates the same fetch stub and setup pattern across the new cases; a small helper would make the regression intent easier to maintain.
  • Add `permissionReadiness: draftDodgePermissionReadiness` to the denied audit metadata in src/queue/processors.ts:3820.
  • Extract a local draft-dodge test setup helper in test/unit/queue.test.ts:11798 that takes installation permissions and optional `getInstallation` behavior.
  • 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 #2134
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:L; 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

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 1, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 40efc5a Commit Preview URL

Branch Preview URL
Jul 01 2026, 11:26 PM

JSONbored added 2 commits July 1, 2026 16:38
The converted_to_draft draft-dodge handler calls closePullRequest
directly instead of routing through executeAgentMaintenanceActions, so
it never got the standard pipeline's PR_WRITE_CLASSES readiness guard.
An installation whose pull_requests: write grant was revoked or never
consented would still attempt the close, get a 403 from GitHub, and
have the failure silently swallowed by the existing .catch() — with
the audit event still recorded as "completed" as if the close actually
happened, giving operators no signal that nothing was mutated.

Add a resolveAgentPermissionReadiness check before the comment/close
calls, mirroring executeAgentMaintenanceActions' own step 6. When not
ready, record a "denied" audit outcome instead of attempting the
GitHub call.
…d of denying

The draft-dodge readiness check's getInstallation call was caught into
null on any failure, which resolveAgentPermissionReadiness then treated
identically to a genuine "pull_requests: write not granted" -- so a
transient D1 read hiccup permanently suppressed the draft-dodge close and
recorded a misleading permission-denied audit instead of letting the
webhook job retry. getInstallation itself never swallows a real read
failure (it only resolves null on a legitimate "row not found" query
result), so removing the added .catch(() => null) lets a genuine failure
propagate: processGitHubWebhook's own top-level catch records the actual
error and re-throws, and the queue's standard retry-on-throw semantics
re-attempt the job once the read succeeds.
@JSONbored
JSONbored force-pushed the claude/draft-dodge-permission-readiness branch from 40efc5a to 1a474eb Compare July 1, 2026 23:40
@JSONbored
JSONbored merged commit 7e3e20e into main Jul 1, 2026
12 checks passed
@JSONbored
JSONbored deleted the claude/draft-dodge-permission-readiness branch July 1, 2026 23:50
@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.

fix(queue): draft-dodge close bypasses the standard actuation pipeline guards

1 participant