Skip to content

fix(review): prevent manual-review label from freezing AI review across head changes - #3550

Closed
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-ai-review-bypass-vulnerability
Closed

fix(review): prevent manual-review label from freezing AI review across head changes#3550
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-ai-review-bypass-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • A sticky manual-review label could cause the agent to reuse a previously published AI review for a different head SHA, allowing stale findings to be applied after a contributor push and creating an authorization bypass risk.
  • The root causes were that getLatestPublishedAiReview ignored head SHA when selecting a published review and the queue logic treated the label itself as a freeze predicate that suppressed fresh review eligibility.

Description

  • Constrain published-review lookups by requiring and checking the current headSha in getLatestPublishedAiReview, and return null for null/absent heads so only same-head published reviews are reused.
  • Remove the manual-label-driven freeze so a sticky manual-review label no longer suppresses the normal head+fingerprint cache / fresh-review path in the queue processor (maybePublishPrPublicSurface).
  • Update tests in test/unit/ai-review-cache.test.ts and test/unit/queue.test.ts to assert that published reviews from different head SHAs are not returned and that a contributor push with a sticky label goes through the fresh-review flow when appropriate.

Testing

  • Ran targeted unit suites with npx vitest run test/unit/ai-review-cache.test.ts test/unit/queue.test.ts -t "maintainer-gated freeze|freeze-owner|LatestPublished|manual-review label alone", and all selected tests passed.
  • Ran npx vitest run test/unit/auto-review-wiring.test.ts and npm run typecheck, both completed successfully.
  • Ran git diff --check which reported no issues; npm audit --audit-level=moderate failed due to a registry 403 Forbidden in this environment (audit request error) and is noted but unrelated to the logic changes.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

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

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-05 16:49:27 UTC

4 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI pending · dirty

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
The diff correctly closes the head-SHA staleness gap: getLatestPublishedAiReview now takes headSha, returns null when it's null/undefined, and the SQL query filters on head_sha so a published review from a different (older) head can no longer be reused. Rather than making the frozen-reuse path head-aware, the PR removes the manual-review-label freeze from maybePublishPrPublicSurface entirely (isFrozenForManualReview is hardcoded to false), which also strips the owner-exemption logic added for the earlier #3476 incident. Tests in both files were updated consistently with the new signature/behavior and the diff is internally coherent with its stated motivation.

Nits — 5 non-blocking
  • processors.ts hardcodes `const isFrozenForManualReview = false;` and leaves stale comments referencing the removed freeze logic instead of deleting the now-dead variable and its downstream threading.
  • Removing the freeze wholesale (rather than just making the frozen-reuse lookup head-aware) also discards the owner/admin/bot exemption logic hardened after the test(review): document the last unreachable review-evasion coverage branch #3476 incident — worth a one-line confirmation that fully retiring the anti-gaming freeze (not just fixing its head-SHA bug) is the intended tradeoff.
  • getLatestPublishedAiReview and the `github_app.ai_review_frozen_reuse` audit event/`gittensory_ai_review_frozen_reuse_total` metric now have no call site outside tests since the only production caller in processors.ts was removed — confirm no dashboards/alerts still depend on that metric before it goes permanently silent.
  • The PR description only links a Codex task, not an eligible open GitHub issue this change closes.
  • Delete the dead `isFrozenForManualReview` constant and its now-unreachable branches/comments in processors.ts rather than pinning it to `false`.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 46 merged, 521 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 56 PR(s), 521 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 56 PR(s), 521 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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

@JSONbored

Copy link
Copy Markdown
Owner Author

Noting for the record, since this closed on merge-conflict grounds rather than the substantive one: this PR's fix hardcodes the manual-review freeze off entirely (isFrozenForManualReview = false) and changes getLatestPublishedAiReview to require an exact head-SHA match, which removes its cross-head reuse purpose. That freeze (with the owner/admin/bot exemption) is deliberate, shipped-today anti-gaming behavior: a PR held for manual review must not let a repeat contributor push buy a fresh AI pass, only an explicit maintainer/collaborator retrigger should. A resubmission of this same fix would reopen that gaming surface and should not be merged as-is.

@JSONbored
JSONbored deleted the codex/propose-fix-for-ai-review-bypass-vulnerability branch July 19, 2026 18:48
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.

Development

Successfully merging this pull request may close these issues.

1 participant