Skip to content

fix(review): verify linked issue closure source - #5233

Merged
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-label-propagation-vulnerability
Jul 12, 2026
Merged

fix(review): verify linked issue closure source#5233
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-label-propagation-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The prior trust predicate accepted any closed linked issue with closed_at >= pr.merged_at, which can be spoofed by an unrelated issue that happens to close after a PR and enable label/reward gaming.
  • The change binds closure authority to GitHub timeline evidence so propagation only trusts an issue when the timeline attributes its close to the specific PR.

Description

  • Add a timeline verifier fetchLinkedIssueClosedByPullRequest in src/github/backfill.ts that reads /issues/{n}/timeline and detects a closing event sourced from the given PR.
  • Replace the weak closedAt >= prMergedAt shortcut with a two-step check in src/review/linked-issue-label-propagation-fetch.ts that requires timeline attribution for timestamp-eligible closed issues.
  • Treat timeline fetch failures as inconclusive (fail-conservative) so transient errors do not silently strip existing labels.
  • Update fetchLinkedIssueLabelsForPropagation to accept and forward prNumber and add regression tests in test/unit/linked-issue-label-propagation-fetch.test.ts covering the legitimate same-PR close path, unrelated post-merge spoofing, missing prNumber context, and timeline fetch failures.

Testing

  • Ran the focused unit suite with npx vitest run test/unit/linked-issue-label-propagation-fetch.test.ts and all tests in that file passed.
  • Ran type checking with npm run typecheck and it passed.
  • Attempted full coverage with npm run test:coverage but the run hit unrelated test/unit/queue.test.ts timeouts and global coverage thresholds failed in this environment, so global coverage was not verified here.
  • Attempted npm audit --audit-level=moderate but the registry audit endpoint returned 403 Forbidden in this environment.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.34%. Comparing base (2e9fab7) to head (e9f0d6e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5233   +/-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files         473      473           
  Lines       39968    39982   +14     
  Branches    14570    14576    +6     
=======================================
+ Hits        37708    37722   +14     
  Misses       1585     1585           
  Partials      675      675           
Flag Coverage Δ
shard-1 46.28% <0.00%> (-0.15%) ⬇️
shard-2 34.60% <50.00%> (+0.13%) ⬆️
shard-3 30.98% <100.00%> (+<0.01%) ⬆️
shard-4 32.91% <0.00%> (-0.40%) ⬇️
shard-5 33.51% <0.00%> (-0.18%) ⬇️
shard-6 45.18% <0.00%> (+0.29%) ⬆️

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

Files with missing lines Coverage Δ
src/github/backfill.ts 97.21% <100.00%> (+0.01%) ⬆️
src/review/linked-issue-label-propagation-fetch.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 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-12 10:09:49 UTC

3 files · 2 AI reviewers · 3 blockers · readiness 93/100 · CI failing · blocked

🛑 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.
  • AI reviewers agree on a likely critical defect: src/github/backfill.ts:3254 only requests `/issues/${issueNumber}/timeline?per_page=100`, so any linked issue with more than 100 timeline events before the closing event will be treated as `not_closed_by_pull_request`
  • paginate until the closing event is found or the timeline is exhausted, or explain why this endpoint is guaranteed to put the close event on page 1. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This PR replaces the timestamp-only trust check (`closedAt >= prMergedAt`) with a two-step check that additionally verifies via the GitHub issue timeline that the close event was actually sourced from this specific PR, closing the spoofing gap where an unrelated issue closing after PR merge could borrow labels. The core logic in `linked-issue-label-propagation-fetch.ts` is sound: `linkedIssueNeedsClosureVerification` gates when a timeline check is needed, `fetchLinkedIssueClosedByPullRequest` fetches `/issues/{n}/timeline` and matches `source.issue.number === prNumber` with a `pull_request` marker, and fetch failures fail-conservative (`inconclusive: true`) rather than silently stripping labels. Tests cover the legitimate close, the spoofing case, missing `prNumber`, and timeline-fetch failure, and the logic traces correctly for each case I checked by hand.

Blockers

  • src/github/backfill.ts:3254 only requests `/issues/${issueNumber}/timeline?per_page=100`, so any linked issue with more than 100 timeline events before the closing event will be treated as `not_closed_by_pull_request`; paginate until the closing event is found or the timeline is exhausted, or explain why this endpoint is guaranteed to put the close event on page 1.
  • PR fix(review): verify linked issue closure source #5233 does not clearly close or link an eligible open issue in this repo; add a `Closes #NNNN` / `Fixes #NNNN` reference to the maintainer-authorized issue or explain why this PR is exempt from the repo issue-scope rule.
Nits — 5 non-blocking
  • `fetchLinkedIssueClosedByPullRequest` in src/github/backfill.ts only fetches `?per_page=100` with no pagination; for an issue with more than 100 preceding timeline events, the actual closing event could fall on a later page, causing a legitimate close to be misclassified as `not_closed_by_pull_request` and silently drop labels.
  • The prior function-level doc comment explaining the anti-gaming rationale (with the fix(review): merging a PR strips its propagated gittensor:priority/feature labels #4528 walkthrough) was removed from `isLinkedIssueTrustworthy` and not replaced with an equivalent explanation of the new two-step verification flow, making the split logic across `linkedIssueNeedsClosureVerification`/`isLinkedIssueTrustworthy`/timeline-fetch harder to follow for the next reader.
  • `isLinkedIssueTrustworthy` recomputes `linkedIssueNeedsClosureVerification(facts, prMergedAt)` a second time even though the caller in `resolveIssueLabelsForPropagation` already computed it — worth passing the boolean through instead of recomputing.
  • Verify all existing callers of `fetchLinkedIssueLabelsForPropagation` were updated to pass `prNumber`; any caller left unmigrated will now have every timestamp-eligible closed issue silently rejected (not just fail-open) since `prNumber === undefined` short-circuits to `{ labels: [], inconclusive: false }`.
  • Consider passing an `until`-style bound or iterating the `Link` header in `fetchLinkedIssueClosedByPullRequest` so timeline events aren't silently truncated on high-activity issues.

Why this is blocked

  • src/github/backfill.ts:3254 only requests `/issues/${issueNumber}/timeline?per_page=100`, so any linked issue with more than 100 timeline events before the closing event will be treated as `not_closed_by_pull_request`; paginate until the closing event is found or the timeline is exhausted, or explain why this endpoint is guaranteed to put the close event on page 1.
  • 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.

CI checks failing

  • validate
  • codecov/patch — 0.00% of diff hit (target 99.00%)
  • validate-tests (2)
Signal Result Evidence
Code review ❌ 3 blockers 2 reviewers, synthesized
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: 45 registered-repo PR(s), 37 merged, 409 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 45 PR(s), 409 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence. LLM value judgment: moderate — The change closes a concrete, previously-flagged spoofing gap (issue #4528) with a targeted timeline-attribution check and adds direct regression tests for the exploit and edge cases, without expanding scope beyond the stated fix.
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: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 45 PR(s), 409 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.
  • 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.
[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 12, 2026
The prior trust predicate accepted any closed linked issue with
closed_at >= pr.merged_at -- spoofable by an unrelated issue that
happens to close after the PR merges. Adds fetchLinkedIssueClosedByPullRequest,
which reads the issue's GitHub timeline and requires the closing event be
attributed to THIS pr number, not just timestamp-eligible. Timeline fetch
failures are treated as inconclusive (fail-conservative), never silently
stripping existing labels.

Also stubs the new /timeline endpoint in queue-5.test.ts's shared
stubPropagationFetch helper: every existing caller there exercises the
legitimate same-PR-close path (never the new spoofing test, which has
its own dedicated stub), so the shared helper can attribute every
closure to the PR number already in scope -- without this, all of
those pre-existing tests silently lost their propagated labels.
@JSONbored
JSONbored force-pushed the codex/propose-fix-for-label-propagation-vulnerability branch from f4e69f4 to e9f0d6e Compare July 12, 2026 10:11
@JSONbored
JSONbored merged commit 5efa953 into main Jul 12, 2026
17 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-label-propagation-vulnerability branch July 12, 2026 10:15
JSONbored added a commit that referenced this pull request Jul 12, 2026
…broken closure check (#5407)

fetchLinkedIssueClosedByPullRequest read GitHub's REST /issues/{n}/timeline looking
for a source.issue field on "closed" events, but that field never appears there --
only on cross-referenced events. Confirmed against three live production issues
(commit_id null, no source key at all). This made the check always fail, turning
a rare race into a 100%-reproducing failure on every "Closes #N" merge. Replaced
with GraphQL's Issue.timelineItems -> ClosedEvent.closer, verified empirically
against live issues.

Also fixes two related gaps found during the same investigation:
- resolvePrTypeLabel picked the exclusive bug/feature label by config array
  order (bug always won when both matched an issue's labels) instead of
  declared precedence. Now the LAST-configured exclusive match wins; operators
  declare exclusive mappings in ascending precedence order. Updated the two
  bundled example configs, which still described the old first-wins rule.
- maybeReReviewOnLinkedIssueChange only checked isConvergenceRepoAllowed,
  unlike the periodic sweep, which also falls back to
  isAgentConfigured(settings.autonomy). Aligned the two gates, short-circuited
  so the common allowlisted case never pays for the extra settings fetch.

Closes #5385
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

Development

Successfully merging this pull request may close these issues.

1 participant