Skip to content

Commit 17bb4a8

Browse files
committed
fix(review): stop the deterministic type-label mislabel from #5233's broken closure check
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
1 parent 0d78f24 commit 17bb4a8

12 files changed

Lines changed: 308 additions & 66 deletions

File tree

.gittensory.yml.example

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -619,8 +619,10 @@ settings:
619619
# output, or existing PR labels, only ever copied from a linked/closing issue ("Fixes #123") that
620620
# ALREADY carries the configured issue label. Generic beyond the priority use case: any issue label
621621
# can map to any PR label. `removeOtherTypeLabels: true` marks the mapping EXCLUSIVE -- it REPLACES
622-
# the type label entirely (bug/feature are removed), for genuinely mutually-exclusive categories; only
623-
# the FIRST-configured exclusive match wins when more than one applies. `false` (e.g. `gittensor:priority`,
622+
# the type label entirely (bug/feature are removed), for genuinely mutually-exclusive categories; when
623+
# more than one applies, the LAST-configured exclusive match wins -- declare exclusive mappings in
624+
# ASCENDING precedence order (lowest-value category first, e.g. bug before feature) so the highest-
625+
# precedence one you actually want is declared last. `false` (e.g. `gittensor:priority`,
624626
# which is a reward tag that coexists WITH whichever type already applies, not a type of its own) applies
625627
# the mapped label ADDITIVELY alongside whichever exclusive match (or the normal title-based bug/feature
626628
# label) already won -- every additive match composes together rather than competing for a single slot.

config/examples/gittensory.full.yml

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -633,8 +633,10 @@ settings:
633633
# output, or existing PR labels, only ever copied from a linked/closing issue ("Fixes #123") that
634634
# ALREADY carries the configured issue label. Generic beyond the priority use case: any issue label
635635
# can map to any PR label. `removeOtherTypeLabels: true` marks the mapping EXCLUSIVE -- it REPLACES
636-
# the type label entirely (bug/feature are removed), for genuinely mutually-exclusive categories; only
637-
# the FIRST-configured exclusive match wins when more than one applies. `false` (e.g. `gittensor:priority`,
636+
# the type label entirely (bug/feature are removed), for genuinely mutually-exclusive categories; when
637+
# more than one applies, the LAST-configured exclusive match wins -- declare exclusive mappings in
638+
# ASCENDING precedence order (lowest-value category first, e.g. bug before feature) so the highest-
639+
# precedence one you actually want is declared last. `false` (e.g. `gittensor:priority`,
638640
# which is a reward tag that coexists WITH whichever type already applies, not a type of its own) applies
639641
# the mapped label ADDITIVELY alongside whichever exclusive match (or the normal title-based bug/feature
640642
# label) already won -- every additive match composes together rather than competing for a single slot.

packages/gittensory-engine/src/settings/pr-type-label.ts

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,9 @@ export type PrTypeLabelDecision = {
114114
/**
115115
* Resolve the TYPE label decision for a PR.
116116
* 1. Linked-issue label PROPAGATION (config-driven, #priority-linked-issue-gate): when enabled, the
117-
* FIRST configured mapping whose `issueLabel` appears (case-insensitively) among the
118-
* ALREADY-FETCHED `linkedIssueLabels` wins. This is the ONLY way a label like `gittensor:priority`
117+
* LAST configured EXCLUSIVE mapping whose `issueLabel` appears (case-insensitively) among the
118+
* ALREADY-FETCHED `linkedIssueLabels` wins (#5385 -- declare exclusive mappings in ascending
119+
* precedence order). This is the ONLY way a label like `gittensor:priority`
119120
* can ever be chosen — this function does no I/O and never infers it from title, changed files,
120121
* AI output, or PR labels; the caller must fetch `linkedIssueLabels` itself (see
121122
* `fetchLinkedIssueLabelsForPropagation` in `review/linked-issue-label-propagation-fetch.ts`).
@@ -152,19 +153,24 @@ export function resolvePrTypeLabel(input: {
152153
if (input.propagation?.enabled) {
153154
const wanted = new Set((input.linkedIssueLabels ?? []).map((label) => label.toLowerCase()));
154155
// Collect EVERY mapping the linked issue's labels satisfy, not just the first. An exclusive mapping
155-
// (removeOtherTypeLabels: true -- e.g. bug/feature, genuinely mutually-exclusive categories) still only
156-
// ever lets the FIRST-configured match win, same precedence as before. But an additive mapping (e.g.
157-
// priority -- a maintainer-hand-picked reward tag that coexists WITH whichever type already applies, not a
158-
// type of its own) must compose with that winner instead of being skipped just because an earlier mapping
159-
// in the array already matched and returned. Before this, an additive match was unreachable whenever the
160-
// SAME linked issue also carried a label an earlier (exclusive) mapping matched -- the overwhelmingly common
161-
// case for gittensor:priority, which is applied ALONGSIDE gittensor:bug/gittensor:feature on the issue, never
162-
// instead of it (#priority-linked-issue-gate).
156+
// (removeOtherTypeLabels: true -- e.g. bug/feature, genuinely mutually-exclusive categories) lets the
157+
// LAST-configured match win, not the first (#5385 fix -- was first-match-wins, which meant a linked issue
158+
// carrying BOTH gittensor:bug and gittensor:feature always resolved to bug, the lower-value label, purely
159+
// because bug is declared before feature in `.gittensory.yml`). Operators must declare exclusive mappings
160+
// in ASCENDING precedence order (lowest-value category first, e.g. bug then feature) so the last match
161+
// encountered while iterating is the highest-precedence one that actually applies -- this mirrors the
162+
// repo's own default mapping order, which is already bug/feature/priority (ascending multiplier value).
163+
// An additive mapping (e.g. priority -- a maintainer-hand-picked reward tag that coexists WITH whichever
164+
// type already applies, not a type of its own) must compose with that winner instead of being skipped just
165+
// because an earlier mapping in the array already matched. Before the original #priority-linked-issue-gate
166+
// composition fix, an additive match was unreachable whenever the SAME linked issue also carried a label an
167+
// earlier (exclusive) mapping matched -- the overwhelmingly common case for gittensor:priority, which is
168+
// applied ALONGSIDE gittensor:bug/gittensor:feature on the issue, never instead of it.
163169
let exclusiveMatch: LinkedIssueLabelPropagationMapping | undefined;
164170
const additiveMatches: LinkedIssueLabelPropagationMapping[] = [];
165171
for (const mapping of input.propagation.mappings) {
166172
if (!wanted.has(mapping.issueLabel.toLowerCase())) continue;
167-
if (mapping.removeOtherTypeLabels) exclusiveMatch ??= mapping;
173+
if (mapping.removeOtherTypeLabels) exclusiveMatch = mapping;
168174
else additiveMatches.push(mapping);
169175
}
170176
if (exclusiveMatch || additiveMatches.length > 0) {

src/github/backfill.ts

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3239,16 +3239,20 @@ export async function fetchLivePullRequestMergedAt(
32393239

32403240
export type LinkedIssueClosureByPullRequestResult = "closed_by_pull_request" | "not_closed_by_pull_request" | "fetch_error";
32413241

3242-
function timelineEventClosesIssueFromPullRequest(
3243-
event: { event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null },
3244-
prNumber: number,
3245-
): boolean {
3246-
return event.event === "closed" && event.source?.issue?.number === prNumber && event.source.issue.pull_request !== undefined;
3247-
}
3248-
3249-
/** Verifies whether GitHub's issue timeline attributes this issue close to the specific PR. Timestamp ordering
3250-
* alone only proves the issue closed after the PR merged; the timeline's closing-reference source binds the
3251-
* closure to THIS PR and prevents borrowing labels from an unrelated issue that happened to close later. */
3242+
/** Verifies whether GitHub attributes this issue's closure to the specific PR, via GraphQL's `ClosedEvent.closer`
3243+
* field -- the one place GitHub's API actually records "what closed this issue" (a `PullRequest` or a `Commit`).
3244+
* Timestamp ordering alone only proves the issue closed after the PR merged; `closer` binds the closure to THIS
3245+
* PR and prevents borrowing labels from an unrelated issue that happened to close later (#4528).
3246+
*
3247+
* #5385 (production incident): a prior version of this check read REST's `GET /issues/{n}/timeline` and looked
3248+
* for a `closed`-type event carrying a `source.issue` field matching this PR. That field NEVER appears on a
3249+
* `closed` event -- confirmed against three live production examples -- it only exists on `cross-referenced`
3250+
* events, a structurally different type. The REST check therefore always returned `not_closed_by_pull_request`,
3251+
* even for a completely legitimate same-PR close, silently converting every "Closes #N" merge into a title-
3252+
* heuristic mislabel. `closer` is GraphQL-only; there is no equivalent REST field to fall back to.
3253+
*
3254+
* `last: 1` deliberately reads only the MOST RECENT closing event, not history: an issue that was closed,
3255+
* reopened, and closed again by something else must be judged on its current closer, not a superseded one. */
32523256
export async function fetchLinkedIssueClosedByPullRequest(
32533257
env: Env,
32543258
repoFullName: string,
@@ -3257,11 +3261,24 @@ export async function fetchLinkedIssueClosedByPullRequest(
32573261
token: string | undefined,
32583262
admissionKey?: GitHubRateLimitAdmissionKey,
32593263
): Promise<LinkedIssueClosureByPullRequestResult> {
3260-
const result = await githubJsonWithHeaders<
3261-
Array<{ event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null }>
3262-
>(env, repoFullName, `/issues/${issueNumber}/timeline?per_page=100`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined);
3264+
if (!token) return "fetch_error";
3265+
const { owner, name } = repoParts(repoFullName);
3266+
if (!owner || !name) return "fetch_error";
3267+
const query = `query GittensoryIssueCloser { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { issue(number: ${issueNumber}) { timelineItems(last: 1, itemTypes: [CLOSED_EVENT]) { nodes { __typename ... on ClosedEvent { closer { __typename ... on PullRequest { number } } } } } } } }`;
3268+
const result = await githubGraphQl<{
3269+
data?: {
3270+
repository?: {
3271+
issue?: { timelineItems?: { nodes?: Array<{ closer?: { __typename?: string | null; number?: number | null } | null } | null> | null } | null } | null;
3272+
} | null;
3273+
};
3274+
errors?: unknown[];
3275+
}>(env, query, token, admissionKey).catch(() => undefined);
32633276
if (result === undefined) return "fetch_error";
3264-
return result.data.some((event) => timelineEventClosesIssueFromPullRequest(event, prNumber)) ? "closed_by_pull_request" : "not_closed_by_pull_request";
3277+
if (Array.isArray(result.errors) && result.errors.length > 0) return "fetch_error";
3278+
const nodes = result.data?.repository?.issue?.timelineItems?.nodes;
3279+
if (!Array.isArray(nodes)) return "fetch_error";
3280+
const closer = nodes[0]?.closer;
3281+
return closer?.__typename === "PullRequest" && closer.number === prNumber ? "closed_by_pull_request" : "not_closed_by_pull_request";
32653282
}
32663283

32673284
/** The issue's LIVE state ("open" / "closed") via REST `GET /issues/{n}`. Mirrors {@link fetchLivePullRequestState}

src/queue/processors.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4086,7 +4086,14 @@ async function maybeReReviewOnLinkedIssueChange(
40864086
const installationId = getInstallationId(payload);
40874087
const issueNumber = payload.issue?.number;
40884088
if (!repoFullName || !installationId || !issueNumber) return false;
4089-
if (isConvergenceRepoAllowed(env, repoFullName)) {
4089+
// #5385: mirrors sweepRepoRegate's own gate exactly -- a repo with acting autonomy configured but NOT in the
4090+
// GITTENSORY_REVIEW_REPOS allowlist (e.g. removed during a rollback, or a self-hoster who configured autonomy
4091+
// without also updating the env allowlist) used to silently never wake affected PRs here, leaving a stale
4092+
// type label (or any other issue-driven verdict) until the sweep eventually reached it, cycles later.
4093+
// Short-circuited deliberately: resolveRepositorySettings does a live manifest fetch, so the allowlisted
4094+
// common case (this repo is already in GITTENSORY_REVIEW_REPOS) must never pay for it -- this handler's own
4095+
// doc comment promises "never doing the expensive live re-review inline", and that includes this gate check.
4096+
if (isConvergenceRepoAllowed(env, repoFullName) || isAgentConfigured((await resolveRepositorySettings(env, repoFullName)).autonomy)) {
40904097
const openPullRequests = await listOpenPullRequests(env, repoFullName);
40914098
// Issue-side label/assignment changes can flip linked-issue hard-rule verdicts from mergeable to close.
40924099
// Wake affected PRs promptly: the issue-side signal can invalidate public gate state, so dropping every

src/settings/pr-type-label.ts

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,9 @@ export type PrTypeLabelDecision = {
114114
/**
115115
* Resolve the TYPE label decision for a PR.
116116
* 1. Linked-issue label PROPAGATION (config-driven, #priority-linked-issue-gate): when enabled, the
117-
* FIRST configured mapping whose `issueLabel` appears (case-insensitively) among the
118-
* ALREADY-FETCHED `linkedIssueLabels` wins. This is the ONLY way a label like `gittensor:priority`
117+
* LAST configured EXCLUSIVE mapping whose `issueLabel` appears (case-insensitively) among the
118+
* ALREADY-FETCHED `linkedIssueLabels` wins (#5385 -- declare exclusive mappings in ascending
119+
* precedence order). This is the ONLY way a label like `gittensor:priority`
119120
* can ever be chosen — this function does no I/O and never infers it from title, changed files,
120121
* AI output, or PR labels; the caller must fetch `linkedIssueLabels` itself (see
121122
* `fetchLinkedIssueLabelsForPropagation` in `review/linked-issue-label-propagation-fetch.ts`).
@@ -152,19 +153,24 @@ export function resolvePrTypeLabel(input: {
152153
if (input.propagation?.enabled) {
153154
const wanted = new Set((input.linkedIssueLabels ?? []).map((label) => label.toLowerCase()));
154155
// Collect EVERY mapping the linked issue's labels satisfy, not just the first. An exclusive mapping
155-
// (removeOtherTypeLabels: true -- e.g. bug/feature, genuinely mutually-exclusive categories) still only
156-
// ever lets the FIRST-configured match win, same precedence as before. But an additive mapping (e.g.
157-
// priority -- a maintainer-hand-picked reward tag that coexists WITH whichever type already applies, not a
158-
// type of its own) must compose with that winner instead of being skipped just because an earlier mapping
159-
// in the array already matched and returned. Before this, an additive match was unreachable whenever the
160-
// SAME linked issue also carried a label an earlier (exclusive) mapping matched -- the overwhelmingly common
161-
// case for gittensor:priority, which is applied ALONGSIDE gittensor:bug/gittensor:feature on the issue, never
162-
// instead of it (#priority-linked-issue-gate).
156+
// (removeOtherTypeLabels: true -- e.g. bug/feature, genuinely mutually-exclusive categories) lets the
157+
// LAST-configured match win, not the first (#5385 fix -- was first-match-wins, which meant a linked issue
158+
// carrying BOTH gittensor:bug and gittensor:feature always resolved to bug, the lower-value label, purely
159+
// because bug is declared before feature in `.gittensory.yml`). Operators must declare exclusive mappings
160+
// in ASCENDING precedence order (lowest-value category first, e.g. bug then feature) so the last match
161+
// encountered while iterating is the highest-precedence one that actually applies -- this mirrors the
162+
// repo's own default mapping order, which is already bug/feature/priority (ascending multiplier value).
163+
// An additive mapping (e.g. priority -- a maintainer-hand-picked reward tag that coexists WITH whichever
164+
// type already applies, not a type of its own) must compose with that winner instead of being skipped just
165+
// because an earlier mapping in the array already matched. Before the original #priority-linked-issue-gate
166+
// composition fix, an additive match was unreachable whenever the SAME linked issue also carried a label an
167+
// earlier (exclusive) mapping matched -- the overwhelmingly common case for gittensor:priority, which is
168+
// applied ALONGSIDE gittensor:bug/gittensor:feature on the issue, never instead of it.
163169
let exclusiveMatch: LinkedIssueLabelPropagationMapping | undefined;
164170
const additiveMatches: LinkedIssueLabelPropagationMapping[] = [];
165171
for (const mapping of input.propagation.mappings) {
166172
if (!wanted.has(mapping.issueLabel.toLowerCase())) continue;
167-
if (mapping.removeOtherTypeLabels) exclusiveMatch ??= mapping;
173+
if (mapping.removeOtherTypeLabels) exclusiveMatch = mapping;
168174
else additiveMatches.push(mapping);
169175
}
170176
if (exclusiveMatch || additiveMatches.length > 0) {

0 commit comments

Comments
 (0)