Skip to content

fix(commands): require an open, non-draft PR for chat's pr_author grant - #5094

Merged
JSONbored merged 1 commit into
mainfrom
fix/chat-pr-author-requires-open-pr
Jul 11, 2026
Merged

fix(commands): require an open, non-draft PR for chat's pr_author grant#5094
JSONbored merged 1 commit into
mainfrom
fix/chat-pr-author-requires-open-pr

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • The per-PR chat rate-limit counter (repoFullName#issueNumber#command) never checks PR state and never resets. Without this fix, a contributor could keep a fresh chat allowance indefinitely by reopening/reusing a closed PR or opening cheap draft PRs -- each PR number gets its own independent, permanent counter.
  • Requires the PR to be open and not draft for a bare pr_author match on chat to authorize, enforced in evaluateCommandAuthorization alongside the existing commandRateLimitPolicy: hold requirement from Allow PR authors to use @gittensory chat when rate limiting is active #5084.
  • Scoped only to the pr_author tier -- maintainers/collaborators are completely unaffected regardless of PR state (the check exists to bound a less-trusted population, not to restrict already-trusted roles).
  • Mirrors into packages/gittensory-engine/src/settings/command-authorization.ts to keep test:engine-parity green.

Note: this doesn't by itself bound "open N genuinely-open non-draft PRs and burn the allowance on each" -- that's a separate, already-existing lever (contributorOpenPrCap), not duplicated here.

Closes #5092

Test plan

  • npm run typecheck
  • Full unsharded npm run test:coverage (14395 passed, 0 failed)
  • New unit tests: the PR-state gate (unset/false/true), maintainers/collaborators unaffected regardless of state
  • New end-to-end tests in queue-5.test.ts: a contributor's own closed PR and own draft PR both deny chat even with commandRateLimitPolicy: hold, with the distinct pr_author_requires_open_pr audit reason
  • npm run command-reference:check / npm run docs:drift-check

The per-PR chat rate-limit counter (repoFullName#issueNumber#command)
never checks PR state and never resets -- a contributor could keep a
fresh chat allowance indefinitely by reopening/reusing a closed PR or
spamming cheap draft PRs, since each PR number gets its own
independent, permanent counter.

Requires the PR to be open and not draft, enforced in
evaluateCommandAuthorization alongside the existing
commandRateLimitPolicy: hold requirement. Scoped only to the pr_author
tier; maintainers/collaborators are unaffected regardless of PR state.

Closes #5092
@superagent-security

Copy link
Copy Markdown
Contributor

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

@cloudflare-workers-and-pages

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 2b0780d Commit Preview URL

Branch Preview URL
Jul 11 2026, 02:39 PM

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.21%. Comparing base (e04e847) to head (2b0780d).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5094   +/-   ##
=======================================
  Coverage   94.20%   94.21%           
=======================================
  Files         470      470           
  Lines       39736    39741    +5     
  Branches    14501    14504    +3     
=======================================
+ Hits        37435    37440    +5     
  Misses       1645     1645           
  Partials      656      656           
Flag Coverage Δ
shard-1 46.42% <53.84%> (+<0.01%) ⬆️
shard-2 33.86% <53.84%> (+<0.01%) ⬆️
shard-3 32.27% <0.00%> (+0.11%) ⬆️
shard-4 31.94% <46.15%> (+0.12%) ⬆️
shard-5 33.53% <0.00%> (-0.01%) ⬇️
shard-6 44.88% <15.38%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...nsory-engine/src/settings/command-authorization.ts 100.00% <100.00%> (ø)
src/github/commands.ts 98.28% <ø> (ø)
src/queue/processors.ts 95.46% <100.00%> (+<0.01%) ⬆️
src/settings/command-authorization.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.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-11 14:45:03 UTC

8 files · 2 AI reviewers · 1 blocker · readiness 100/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • AI reviewers agree on a likely critical defect: src/queue/processors.ts:12565 wires `pullRequestOpenAndNotDraft` from `cachedPullRequest`, so a dropped/out-of-order close or draft-conversion webhook leaves a stale open/non-draft cache entry that still authorizes `chat` on a closed or draft PR
  • change this to derive the open check from the current issue payload and refresh the PR draft state from GitHub or an existing live-PR helper before authorizing. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This PR closes the loophole where a contributor could keep an indefinite chat allowance by reopening a closed PR or spamming draft PRs, since the per-PR rate-limit counter never resets or checks PR state. It adds a `pullRequestOpenAndNotDraft` gate in `evaluateCommandAuthorization` that fires only for the already-narrow `pr_author` + `PR_AUTHOR_RATE_LIMITED_COMMANDS` (chat) path, leaving maintainers/collaborators untouched, and the reordering of the rate-limit vs matchedRole checks correctly preserves the pre-existing `pr_author_requires_rate_limiting` denial semantics. The caller wiring in `src/queue/processors.ts` computes the flag directly from `cachedPullRequest.state`/`isDraft`, and both engine/non-engine copies plus tests are kept in parity.

Blockers

  • src/queue/processors.ts:12565 wires `pullRequestOpenAndNotDraft` from `cachedPullRequest`, so a dropped/out-of-order close or draft-conversion webhook leaves a stale open/non-draft cache entry that still authorizes `chat` on a closed or draft PR; change this to derive the open check from the current issue payload and refresh the PR draft state from GitHub or an existing live-PR helper before authorizing.
Nits — 6 non-blocking
  • The `cachedPullRequest?.state === "open" && cachedPullRequest?.isDraft !== true` expression in src/queue/processors.ts relies on `cachedPullRequest` being freshly refreshed at command-processing time — worth double-checking (not visible in this diff) that this cached record reflects the live PR state at comment time rather than a stale webhook-time snapshot, given the file's own review-evasion module elsewhere treats cache staleness as a real hazard.
  • The external brief flags two generic_secret_assignment matches in test/unit/queue-5.test.ts (lines ~1390/1426) — these are almost certainly the `GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem()` test fixture calls already used identically elsewhere in the file, not real secrets, but worth a quick confirm.
  • `prAuthorGatedCommand` line in both command-authorization.ts copies exceeds the file's apparent line-length convention (unwrapped boolean expression) compared to the multi-line style used elsewhere in the same function — minor style nit.
  • Consider adding a short code comment or test asserting that `cachedPullRequest` is refetched (not served from a long-lived cache) immediately before this authorization check, to guard against the exact kind of stale-vs-live bug this codebase's review guide calls out as high-blast-radius.
  • The two nearly-identical `command-authorization.ts` files (engine + non-engine) and their test mirrors suggest a shared-source risk if only one copy is updated in a future change — consider a lint/build check enforcing parity if `test:engine-parity` doesn't already fully cover this function.
  • 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.

Concerns raised — review before merging

  • src/queue/processors.ts:12565 wires `pullRequestOpenAndNotDraft` from `cachedPullRequest`, so a dropped/out-of-order close or draft-conversion webhook leaves a stale open/non-draft cache entry that still authorizes `chat` on a closed or draft PR; change this to derive the open check from the current issue payload and refresh the PR draft state from GitHub or an existing live-PR helper before authorizing.
Signal Result Evidence
Code review ❌ 1 blocker 2 reviewers, synthesized
Linked issue ✅ Linked #5092
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 46 registered-repo PR(s), 38 merged, 416 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 46 PR(s), 416 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Linked issue satisfaction

Addressed
The diff adds a pullRequestOpenAndNotDraft gate in evaluateCommandAuthorization that denies a bare pr_author match on chat with the distinct reason pr_author_requires_open_pr unless the PR is open and non-draft, wires it through commands.ts/processors.ts using cachedPullRequest.state/isDraft (covering closed/merged, which GitHub reports as state="closed"), and leaves maintainer/collaborator author

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 46 PR(s), 416 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 the manual-review Gittensor contributor context label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

An AI reviewer flagged a likely defect, but its confidence was below this repository's configured close-confidence floor, so this is held for a maintainer to confirm instead of closing automatically. Resolve the flagged defect (see the review notes), or ask a maintainer to override.

@JSONbored
JSONbored merged commit eb3d641 into main Jul 11, 2026
20 checks passed
@JSONbored
JSONbored deleted the fix/chat-pr-author-requires-open-pr branch July 11, 2026 14:47
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chat's pr_author grant must require an open, non-draft PR

1 participant