diff --git a/docs/docs/features/agents-and-models.md b/docs/docs/features/agents-and-models.md index 055d86cbd..2b5ddcb5e 100644 --- a/docs/docs/features/agents-and-models.md +++ b/docs/docs/features/agents-and-models.md @@ -84,7 +84,7 @@ The same aliases work in PR comments (the `llm-` prefix is optional; the raw cat ``` /switch claude-opus5 # future follow-ups on this PR use this model -/use codex-gpt56-sol # one follow-up with this model +/use codex-gpt56-sol # switch the PR and run one follow-up with this model /review claude-opus5 codex-gpt56-sol # independent reviews from two models ``` diff --git a/docs/docs/features/pr-commands.md b/docs/docs/features/pr-commands.md index bea863978..8493b6bce 100644 --- a/docs/docs/features/pr-commands.md +++ b/docs/docs/features/pr-commands.md @@ -23,7 +23,7 @@ To **take over an existing PR** for ongoing work (so that natural follow-up comm | `/fix` | You want to apply a `/review`'s pending suggestions | Yes | [`/fix`](#fix) | | `/merge` | You want the base branch merged into the PR branch | Maybe, if conflicts need resolution | [`/merge`](#merge) | | `/switch ` | You want future PR work to use a different model | No, unless you include follow-up instructions | [`/switch`](#switch) | -| `/use ` | You want one immediate follow-up run with a temporary model | Yes | [`/use`](#use) | +| `/use ` | You want to switch models and run an immediate follow-up | Yes | [`/use`](#use) | | `/ultrafix` | You want an automated review-fix loop | Yes | [`/ultrafix`](#ultrafix) | ## Syntax Rules @@ -170,14 +170,14 @@ Without instructions, `/switch` only updates the label and makes no code changes ### `/use` -`/use` runs one immediate follow-up task with a temporary model: +`/use` is the documented switch-and-run command. It first replaces the PR's model label with the selected model's canonical configured label, then queues an immediate follow-up: ```text /use Please investigate the flaky test failure and update the PR. ``` -The PR's model label keeps its current value. Later work returns to the PR's configured model unless you use `/switch` or another `/use`. Like `/switch`, `/use` takes one model argument, and the agent sees only your instructions, without the command syntax. +The label update completes before work is queued, and the selected agent and model are also stored on the job. This keeps batching, restarts, and provider-limit retries on the new provider. If another writer is active, ProPR saves the follow-up for the next run instead of starting a concurrent writer. A delayed provider-limit retry is superseded so the new provider does not wait for the old provider's reset. Like `/switch`, `/use` takes one model argument, and the agent sees only your instructions, without the command syntax. ### Choosing A Model @@ -187,7 +187,7 @@ Use routing when: - A model is better suited to the task - The current model is stuck -- You want a one-off second opinion +- You want to move the PR to a different provider - You need to work around provider capacity or rate limits ## Ultrafix And Branch Updates diff --git a/docs/docs/operations/troubleshooting.md b/docs/docs/operations/troubleshooting.md index aaddd6ba7..2755153d9 100644 --- a/docs/docs/operations/troubleshooting.md +++ b/docs/docs/operations/troubleshooting.md @@ -99,7 +99,7 @@ Then check credentials, branch settings, and agent configuration — the usual c Recovery runs through the PR conversation: - Add a clearer follow-up comment with stronger instructions. -- `/switch ` to change the PR's model going forward, or `/use ` for a one-off task with a different model. +- `/switch ` to change only the PR's model label, or `/use ` to change the durable label and immediately retry the follow-up with that model. - `/review` then `/fix`, or `/ultrafix` for an automated review-fix loop (remove the `ultrafix` PR label to stop it). - Re-run with a smaller scope — see [Work Splitting](../features/work-splitting.md). - Undo a bad commit with `propr task revert owner/repo `, which runs a signed system task (authorized via `SYSTEM_TASK_SECRET`) that resets the branch and force-pushes. diff --git a/docs/docs/tutorials/usage.md b/docs/docs/tutorials/usage.md index e29c179e4..88cb61f4a 100644 --- a/docs/docs/tutorials/usage.md +++ b/docs/docs/tutorials/usage.md @@ -79,7 +79,7 @@ Use slash commands only for specific actions: - `/fix` applies unprocessed AI review comments generated by `/review`. - `/merge` merges the base branch into the PR branch, attempts automatic conflict resolution, and reports back. - `/switch ` changes the PR's model label going forward. -- `/use ` runs one follow-up task with that model without changing the PR's model. +- `/use ` switches the PR's durable model label and runs one follow-up task with that model. - `/ultrafix` runs a review-fix loop. Parameters: `goal=`, `max=`, `pause=`, `model=`, for example `/ultrafix goal=9 max=5`. It waits for CI checks and PR inactivity between cycles. The `ultrafix` PR label is the circuit breaker — remove it to stop the loop. See [PR Slash Commands](../features/pr-commands.md). diff --git a/packages/core/src/config/modelAliases.ts b/packages/core/src/config/modelAliases.ts index 7fbca9938..3620d49f5 100644 --- a/packages/core/src/config/modelAliases.ts +++ b/packages/core/src/config/modelAliases.ts @@ -366,7 +366,10 @@ export { getAllCustomLabels, resolveCustomLabel, resolveLlmLabel, + resolveCanonicalModelSelection, + resolveCanonicalModelSelectionFromLabels, resolveReviewModels, + type CanonicalModelSelection, ReviewModelResolutionError, type ReviewAssignment, } from './modelLabelResolution.js'; diff --git a/packages/core/src/config/modelLabelResolution.ts b/packages/core/src/config/modelLabelResolution.ts index 3e1947e03..3891391a5 100644 --- a/packages/core/src/config/modelLabelResolution.ts +++ b/packages/core/src/config/modelLabelResolution.ts @@ -2,6 +2,7 @@ import { AgentRegistry } from '../agents/AgentRegistry.js'; import type { AgentConfig } from '../agents/types.js'; import { toProprOpenCodeModelId } from '../agents/impl/openCodeModelIds.js'; import { shortHash } from '@propr/shared'; +import { buildAgentModelLlmLabel, buildDynamicLlmLabel } from '@propr/shared'; import { ALL_MODELS, MODEL_INFO_MAP, type AgentType } from './modelDefinitions.js'; import { MODEL_ALIASES, @@ -30,6 +31,7 @@ async function resolveCustomLabel(label: string): Promise { + const token = requested.trim(); + if (!token) return null; + + const registry = AgentRegistry.getInstance(); + await registry.ensureInitialized(); + const agents = registry.getAllAgents(); + + // parseSlashCommand strips llm-, while callers outside the parser may pass + // a complete configured label. Try both spellings for custom labels. + const customCandidates = token.toLowerCase().startsWith('llm-') + ? [token] + : [token, `llm-${token}`]; + let customResolution: LlmLabelResolution | null = null; + let matchedCustomLabel: string | null = null; + for (const candidate of customCandidates) { + customResolution = await resolveCustomLabel(candidate); + if (customResolution) { + matchedCustomLabel = candidate; + break; + } + } + + const normalizedToken = token.replace(/^llm-/i, ''); + const resolution = customResolution ?? await resolveLlmLabel(normalizedToken, true); + const agent = agents.find(candidate => + candidate.config.enabled + && candidate.config.alias.toLowerCase() === resolution.agentAlias.toLowerCase() + ); + if (!agent) return null; + + const configuredModel = agent.config.supportedModels.find(model => + model.toLowerCase() === resolution.model.toLowerCase() + ); + if (!configuredModel) return null; + + const configuredCustomLabel = agent.config.modelCustomLabels?.[configuredModel] + ?? Object.entries(agent.config.modelCustomLabels ?? {}).find( + ([model]) => model.toLowerCase() === configuredModel.toLowerCase() + )?.[1]; + const modelInfo = MODEL_INFO_MAP[configuredModel]; + const labelAgentAlias = agent.config.alias === 'default' + ? agent.config.type + : agent.config.alias; + const githubLabel = configuredCustomLabel + || matchedCustomLabel + || (modelInfo && buildAgentModelLlmLabel(agent.config.type, labelAgentAlias, modelInfo)) + || buildDynamicLlmLabel(agent.config.alias, configuredModel); + + return { agentAlias: agent.config.alias, model: configuredModel, githubLabel }; +} + +/** Resolve the canonical configured selection represented by a PR label set. */ +async function resolveCanonicalModelSelectionFromLabels( + labels: Array, + modelLabelPattern = '^llm-(.+)$', +): Promise { + const pattern = new RegExp(modelLabelPattern); + const customLabels = new Set((await getAllCustomLabels()).map(label => label.toLowerCase())); + const managedLabels: string[] = []; + for (const label of labels) { + const name = typeof label === 'string' ? label : label.name; + if (customLabels.has(name.toLowerCase()) || name.match(pattern)?.[1]) managedLabels.push(name); + } + // Exclusive label convergence adds the target before removing the old + // label. Do not choose either routing while that transition is visible. + if (managedLabels.length !== 1) return null; + + const [managedLabel] = managedLabels; + const requested = customLabels.has(managedLabel.toLowerCase()) + ? managedLabel + : managedLabel.match(pattern)?.[1] ?? managedLabel; + return resolveCanonicalModelSelection(requested); +} + /** * Gets all custom labels configured across all models in all agents. * @@ -265,11 +353,14 @@ function resolveKnownModelAliasLabel(label: string, agents: { config: AgentConfi * @param label - The LLM label without the "llm-" prefix (e.g., "gemini-pro", "claude-opus", "opus") * @returns Object with agentAlias and model */ -async function resolveLlmLabel(label: string): Promise { +async function resolveLlmLabel(label: string, enabledOnly = false): Promise { const registry = AgentRegistry.getInstance(); await registry.ensureInitialized(); - const agents = registry.getAllAgents(); + const agents = registry.getAllAgents().filter(agent => !enabledOnly || agent.config.enabled); + const defaultAgentAlias = enabledOnly + ? agents[0]?.config.alias ?? 'default' + : registry.getDefaultAgent()?.config.alias ?? 'default'; const supportedModelMatch = resolveBySupportedModelId(label, agents); if (supportedModelMatch) { @@ -282,8 +373,7 @@ async function resolveLlmLabel(label: string): Promise { return explicitLabelMatch; } if (label.includes('~')) { - const defaultAgent = registry.getDefaultAgent(); - return { agentAlias: defaultAgent?.config.alias || 'default', model: label }; + return { agentAlias: defaultAgentAlias, model: label }; } const lowerLabel = label.toLowerCase(); @@ -323,8 +413,7 @@ async function resolveLlmLabel(label: string): Promise { return knownAliasMatch; } - const defaultAgent = registry.getDefaultAgent(); - return { agentAlias: defaultAgent?.config.alias || 'default', model: label }; + return { agentAlias: defaultAgentAlias, model: label }; } /** @@ -455,5 +544,7 @@ export { getAllCustomLabels, resolveCustomLabel, resolveLlmLabel, + resolveCanonicalModelSelection, + resolveCanonicalModelSelectionFromLabels, resolveReviewModels, }; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index a0450c8fd..43ec23f22 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -25,6 +25,12 @@ export { formatResetTime, addModelSpecificDelay, parseResetTimeFromMessage, calc export { filterCommentByAuthor, checkCommentTrigger, checkCommentIgnore } from './utils/commentFilters.js'; export { ensureGitRepository } from './utils/git/gitValidation.js'; export { safeRemoveLabel, safeAddLabel, safeUpdateLabels } from './utils/github/labelOperations.js'; +export { + getUnprocessedCommentIdentity, + getUnprocessedCommentRevisionIdentity, + dedupeUnprocessedComments, + restorePendingCommentsIdempotently, +} from './utils/pendingComments.js'; export type { LabelContext, UpdateResults } from './utils/github/labelOperations.js'; export { createLogFiles, generateCompletionComment, redactSecrets } from './utils/github/logFiles.js'; export { formatSubscriptionUsage } from './utils/github/formatSubscriptionUsage.js'; @@ -67,7 +73,7 @@ export type { PlanIssueDefaultSelection } from './config/planIssueDefaultSelecti export { getPlanIssueDefaultSelection } from './config/planIssueDefaultSelection.js'; export type { PlanIssueSelectionAgent } from './config/planIssueDefaultSelection.js'; export { resolveConfiguredModel } from './config/configuredModel.js'; -export { resolveModelAlias, getDefaultModel, getPreferredModelForAgent, getModelShortName, getModelName, MODEL_ALIASES, MODEL_SHORT_NAMES, resolveLlmLabel, getOpenRouterId, getAgentTypeFromModel, resolveCustomLabel, getAllCustomLabels, findMatchingModel, resolveReviewModels, ReviewModelResolutionError, NoDefaultModelConfiguredError } from './config/modelAliases.js'; +export { resolveModelAlias, getDefaultModel, getPreferredModelForAgent, getModelShortName, getModelName, MODEL_ALIASES, MODEL_SHORT_NAMES, resolveLlmLabel, resolveCanonicalModelSelection, resolveCanonicalModelSelectionFromLabels, getOpenRouterId, getAgentTypeFromModel, resolveCustomLabel, getAllCustomLabels, findMatchingModel, resolveReviewModels, ReviewModelResolutionError, NoDefaultModelConfiguredError, type CanonicalModelSelection } from './config/modelAliases.js'; export type { LlmLabelResolution, ReviewAssignment } from './config/modelAliases.js'; export { CLAUDE_MODELS, CODEX_MODELS, ANTIGRAVITY_MODELS, OPENCODE_MODELS, VIBE_MODELS, ALL_MODELS, AGENT_MODELS, AGENT_DISPLAY, AGENT_DISPLAY_ORDER, MODEL_INFO_MAP, AGENT_DEFAULTS, typeBadgeColors } from './config/modelDefinitions.js'; export type { AgentType as ModelAgentType, AgentDisplayInfo, ModelInfo } from './config/modelDefinitions.js'; diff --git a/packages/core/src/queue/taskQueue.types.ts b/packages/core/src/queue/taskQueue.types.ts index 09058f621..5aa1429d0 100644 --- a/packages/core/src/queue/taskQueue.types.ts +++ b/packages/core/src/queue/taskQueue.types.ts @@ -48,6 +48,12 @@ export interface CommentJobData { repoOwner: string; repoName: string; llm?: string | null; + /** Durable explicit routing selected by /use or /switch. */ + agentAlias?: string; + modelName?: string; + modelLabel?: string; + /** Marks the reconstructable delayed job created after a provider limit. */ + isRetryFromRateLimit?: boolean; correlationId: string; title?: string; subtitle?: string; @@ -65,6 +71,10 @@ export interface CommentJobData { commandCommentId?: number; /** Creation time of the GitHub comment that established the queued command context. */ commandCommentCreatedAt?: string; + /** Revision time of the GitHub comment that established the queued command context. */ + commandCommentUpdatedAt?: string; + /** Timestamp-and-body identity of the revision that established the queued command context. */ + commandCommentRevisionIdentity?: string; /** GitHub resource type of the comment that established the queued command context. */ commandCommentType?: 'review' | 'issue'; /** Ultrafix-specific settings when commandMode is 'ultrafix' */ @@ -79,6 +89,10 @@ export interface UnprocessedComment { id: number; /** GitHub creation time used to order issue and review comments together. */ createdAt?: string; + /** GitHub revision time used to distinguish edited comment deliveries. */ + updatedAt?: string; + /** Stable timestamp-and-body identity used across webhook, queue, and retry storage. */ + revisionIdentity?: string; body: string; body_html?: string; // HTML with signed image URLs (from accept: application/vnd.github.full+json) author: string; @@ -90,6 +104,10 @@ export interface UnprocessedComment { requestedModels?: string[]; commandInstructions?: string; llmOverride?: string | null; + /** Explicit routing carried through pending-comment batching. */ + agentAlias?: string; + modelName?: string; + modelLabel?: string; /** Ultrafix-specific settings when commandMode is 'ultrafix' */ ultrafixMeta?: UltrafixCommandMeta; } diff --git a/packages/core/src/utils/github/labelOperations.ts b/packages/core/src/utils/github/labelOperations.ts index 02e33a6e5..4779f0ff6 100644 --- a/packages/core/src/utils/github/labelOperations.ts +++ b/packages/core/src/utils/github/labelOperations.ts @@ -1,4 +1,6 @@ import type { Logger } from 'pino'; +import type { Redis } from 'ioredis'; +import { LabelTransitionLeaseError, withLabelTransitionLease, type LabelTransitionLease } from '../ultrafixLabelTransition.js'; interface OctokitLike { request: (endpoint: string, options: Record) => Promise; @@ -14,9 +16,243 @@ export interface LabelContext { export interface UpdateResults { success: boolean; + /** The transition was superseded before any labels were mutated. */ + skipped?: boolean; removed: string[]; added: string[]; errors: string[]; + /** Labels observed by the final live verification read. */ + finalLabels?: string[]; +} + +export interface ExclusiveLabelConvergence { + /** The one managed label that must remain after convergence. */ + targetLabel: string; + /** Classifies labels owned by this transition. Unmanaged labels are never mutated. */ + isManagedLabel: (labelName: string) => boolean; + /** Maximum live-read/mutate/verify attempts. */ + maxAttempts?: number; + /** Redis client used to serialize this PR's complete exclusive transition. */ + redis: Pick; + /** Claim this transition while its per-PR lease is held. */ + claimTransition?: () => Promise; + /** The caller's verified lease when publication extends beyond convergence. */ + lease?: LabelTransitionLease; +} + +interface IssueLabelsResponse { + data: { labels?: Array }; +} + +interface ManagedLabelRestoration { + priorManagedLabels: string[]; + results: UpdateResults; + lease: LabelTransitionLease; +} + +function labelNames(response: IssueLabelsResponse): string[] { + return (response.data.labels ?? []).flatMap(label => + typeof label === 'string' ? [label] : label.name ? [label.name] : []); +} + +async function readLiveLabels(context: LabelContext): Promise { + const response = await context.octokit.request( + 'GET /repos/{owner}/{repo}/issues/{issue_number}', + { owner: context.owner, repo: context.repo, issue_number: context.issueNumber }, + ); + return labelNames(response); +} + +async function restoreManagedLabels( + context: LabelContext, + convergence: ExclusiveLabelConvergence, + restoration: ManagedLabelRestoration, +): Promise { + const { priorManagedLabels, results, lease } = restoration; + await lease.assertOwned(); + const priorLabelNames = new Set(priorManagedLabels.map(label => label.toLowerCase())); + let liveLabels: string[]; + try { + liveLabels = await readLiveLabels(context); + } catch (error) { + results.errors.push(`Failed to read live labels for restoration: ${(error as Error).message}`); + return; + } + + const transitionLabelNames = new Set([ + ...priorManagedLabels.map(label => label.toLowerCase()), + convergence.targetLabel.toLowerCase(), + ]); + const liveManagedLabels = liveLabels.filter(convergence.isManagedLabel); + const newerSingletonSelection = liveManagedLabels.length === 1 + && !transitionLabelNames.has(liveManagedLabels[0].toLowerCase()) + ? liveManagedLabels[0] + : undefined; + if (newerSingletonSelection) { + // Another transition has established a valid singleton selection that + // this failed attempt never owned. Restoring our snapshot would + // overwrite that newer durable source of truth. + results.finalLabels = liveLabels; + results.errors.push( + `Skipped model-label restoration because live selection changed to: ${newerSingletonSelection}`, + ); + context.logger.warn({ + issueNumber: context.issueNumber, + targetLabel: convergence.targetLabel, + newerSingletonSelection, + }, 'Skipped stale model-label restoration after a concurrent transition'); + return; + } + + for (const label of priorManagedLabels) { + if (liveLabels.some(liveLabel => liveLabel.toLowerCase() === label.toLowerCase())) continue; + await lease.assertOwned(); + const added = await safeAddLabel(context, label); + await lease.assertOwned(); + if (added) results.added.push(label); + else results.errors.push(`Failed to restore '${label}'`); + } + + await lease.assertOwned(); + try { + liveLabels = await readLiveLabels(context); + } catch (error) { + results.errors.push(`Failed to verify restored model labels: ${(error as Error).message}`); + return; + } + + const allPriorLabelsPresent = priorManagedLabels.every(label => + liveLabels.some(liveLabel => liveLabel.toLowerCase() === label.toLowerCase())); + if (!allPriorLabelsPresent) { + results.finalLabels = liveLabels; + results.errors.push('Could not restore the prior model-label set'); + return; + } + + for (const label of liveLabels.filter(convergence.isManagedLabel)) { + if (priorLabelNames.has(label.toLowerCase())) continue; + await lease.assertOwned(); + const removed = await safeRemoveLabel(context, label); + await lease.assertOwned(); + if (removed) results.removed.push(label); + else results.errors.push(`Failed to remove partial transition label '${label}' during restoration`); + } + + await lease.assertOwned(); + try { + results.finalLabels = await readLiveLabels(context); + const restoredManagedLabels = results.finalLabels.filter(convergence.isManagedLabel); + const restoredExactly = restoredManagedLabels.length === priorManagedLabels.length + && restoredManagedLabels.every(label => priorLabelNames.has(label.toLowerCase())); + if (!restoredExactly) results.errors.push('Prior model-label set was not restored exactly'); + } catch (error) { + results.errors.push(`Failed to verify final restored labels: ${(error as Error).message}`); + } +} + +async function convergeExclusiveLabel( + context: LabelContext, + convergence: ExclusiveLabelConvergence, + results: UpdateResults, + lease: LabelTransitionLease, +): Promise { + const { issueNumber, logger } = context; + const maxAttempts = Math.max(1, convergence.maxAttempts ?? 3); + const targetLower = convergence.targetLabel.toLowerCase(); + let priorManagedLabels: string[] | undefined; + + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + // Fence every attempt as well as every mutation. If ownership expires + // during a GitHub request, the post-mutation check prevents this stale + // transition from issuing any further managed-label writes. + await lease.assertOwned(); + let liveLabels: string[]; + try { + liveLabels = await readLiveLabels(context); + } catch (error) { + const message = (error as Error).message; + results.errors.push(`Attempt ${attempt}: failed to read live labels: ${message}`); + logger.warn({ error: message, issueNumber, attempt }, 'Failed to read live labels before model-label transition'); + continue; + } + + const managedLabels = liveLabels.filter(convergence.isManagedLabel); + priorManagedLabels ??= managedLabels; + const targetPresent = managedLabels.some(label => label.toLowerCase() === targetLower); + let targetEstablished = targetPresent; + if (!targetPresent) { + await lease.assertOwned(); + targetEstablished = await safeAddLabel(context, convergence.targetLabel); + await lease.assertOwned(); + if (targetEstablished) { + results.added.push(convergence.targetLabel); + } else { + results.errors.push(`Attempt ${attempt}: failed to add '${convergence.targetLabel}'; prior model labels were retained`); + } + } + + const labelsToRemove = targetEstablished + ? managedLabels.filter(label => label.toLowerCase() !== targetLower) + : []; + for (const label of labelsToRemove) { + await lease.assertOwned(); + const removed = await safeRemoveLabel(context, label); + await lease.assertOwned(); + if (removed) results.removed.push(label); + else results.errors.push(`Attempt ${attempt}: failed to remove '${label}'`); + } + + await lease.assertOwned(); + try { + const verifiedLabels = await readLiveLabels(context); + results.finalLabels = verifiedLabels; + const verifiedManagedLabels = verifiedLabels.filter(convergence.isManagedLabel); + if ( + verifiedManagedLabels.length === 1 + && verifiedManagedLabels[0].toLowerCase() === targetLower + ) { + results.success = true; + return; + } + results.errors.push( + `Attempt ${attempt}: model-label invariant not satisfied (found: ${verifiedManagedLabels.join(', ') || 'none'})`, + ); + } catch (error) { + const message = (error as Error).message; + results.errors.push(`Attempt ${attempt}: failed to verify live labels: ${message}`); + logger.warn({ error: message, issueNumber, attempt }, 'Failed to verify live labels after model-label transition'); + } + } + + results.success = false; + if (priorManagedLabels !== undefined) { + await restoreManagedLabels(context, convergence, { priorManagedLabels, results, lease }); + } +} + +async function runExclusiveLabelTransition( + context: LabelContext, + convergence: ExclusiveLabelConvergence, + transition: (lease: LabelTransitionLease) => Promise, +): Promise { + if (!convergence.lease) { + await withLabelTransitionLease( + convergence.redis, + { owner: context.owner, repo: context.repo, pr: context.issueNumber }, + transition, + ); + return; + } + + const { identity } = convergence.lease; + if ( + identity.owner !== context.owner + || identity.repo !== context.repo + || identity.pr !== context.issueNumber + ) throw new LabelTransitionLeaseError('PR label transition lease identity does not match label context'); + await convergence.lease.assertOwned(); + await transition(convergence.lease); + await convergence.lease.assertOwned(); } export async function safeRemoveLabel(context: LabelContext, labelName: string): Promise { @@ -73,8 +309,16 @@ export async function safeAddLabel(context: LabelContext, labelName: string): Pr } } -export async function safeUpdateLabels(context: LabelContext, labelsToRemove: string[] = [], labelsToAdd: string[] = []): Promise { +export async function safeUpdateLabels( + context: LabelContext, + labelsToRemove: string[] = [], + labelsToAdd: string[] = [], + /** Replace a snapshot, or converge one managed label exclusively from live reads. */ + currentLabelsOrConvergence?: string[] | ExclusiveLabelConvergence, +): Promise { const { issueNumber, logger } = context; + const currentLabels = Array.isArray(currentLabelsOrConvergence) ? currentLabelsOrConvergence : undefined; + const convergence = Array.isArray(currentLabelsOrConvergence) ? undefined : currentLabelsOrConvergence; const results: UpdateResults = { success: true, removed: [], @@ -82,7 +326,48 @@ export async function safeUpdateLabels(context: LabelContext, labelsToRemove: st errors: [] }; - for (const labelName of labelsToRemove) { + if (convergence) { + // The model-selection path deliberately avoids PUT of a complete label + // set: only labels classified as model labels may be touched. Its lease + // covers the initial snapshot, convergence, verification, and rollback. + results.success = false; + try { + const transition = async (lease: LabelTransitionLease): Promise => { + if (convergence.claimTransition && !await convergence.claimTransition()) { + results.skipped = true; + return; + } + await convergeExclusiveLabel(context, convergence, results, lease); + }; + await runExclusiveLabelTransition(context, convergence, transition); + } catch (error) { + const message = (error as Error).message; + results.success = false; + results.errors.push(`Failed to hold PR label transition lease: ${message}`); + logger.warn({ error: message, issueNumber }, 'Exclusive label transition lease failed'); + } + } else if (currentLabels) { + const removedNames = new Set(labelsToRemove.map(label => label.toLowerCase())); + const desiredLabels = currentLabels.filter(label => !removedNames.has(label.toLowerCase())); + for (const label of labelsToAdd) { + if (!desiredLabels.some(existing => existing.toLowerCase() === label.toLowerCase())) desiredLabels.push(label); + } + try { + await context.octokit.request('PUT /repos/{owner}/{repo}/issues/{issue_number}/labels', { + owner: context.owner, + repo: context.repo, + issue_number: issueNumber, + labels: desiredLabels, + }); + results.removed.push(...labelsToRemove); + results.added.push(...labelsToAdd); + } catch (error) { + const err = error as Error & { status?: number }; + results.success = false; + results.errors.push(`Failed to atomically replace labels: ${err.message}`); + logger.warn({ error: err.message, issueNumber, status: err.status }, 'Failed to atomically replace issue labels'); + } + } else for (const labelName of labelsToRemove) { const removed = await safeRemoveLabel(context, labelName); if (removed) { results.removed.push(labelName); @@ -92,7 +377,7 @@ export async function safeUpdateLabels(context: LabelContext, labelsToRemove: st } } - for (const labelName of labelsToAdd) { + if (!convergence && !currentLabels) for (const labelName of labelsToAdd) { const added = await safeAddLabel(context, labelName); if (added) { results.added.push(labelName); diff --git a/packages/core/src/utils/pendingComments.ts b/packages/core/src/utils/pendingComments.ts new file mode 100644 index 000000000..76e69ce27 --- /dev/null +++ b/packages/core/src/utils/pendingComments.ts @@ -0,0 +1,85 @@ +import type { Redis } from 'ioredis'; +import { createHash } from 'node:crypto'; +import type { UnprocessedComment } from '../queue/taskQueue.types.js'; + +export function getUnprocessedCommentRevisionIdentity( + comment: Pick, +): string { + if (comment.revisionIdentity) return comment.revisionIdentity; + const revision = comment.updatedAt ?? comment.createdAt ?? ''; + const bodyDigest = createHash('sha256').update(`${comment.type}\0${comment.body}`).digest('hex').slice(0, 12); + return `${revision}:${bodyDigest}`; +} + +/** Identity is namespaced because issue and review comments have independent ID sequences. */ +export function getUnprocessedCommentIdentity(comment: UnprocessedComment): string { + return `${comment.type}:${comment.id}:${getUnprocessedCommentRevisionIdentity(comment)}`; +} + +/** Preserve first-seen order and the complete first payload for each comment revision. */ +export function dedupeUnprocessedComments(comments: UnprocessedComment[]): UnprocessedComment[] { + const seen = new Set(); + return comments.filter(comment => { + const identity = getUnprocessedCommentIdentity(comment); + if (seen.has(identity)) return false; + seen.add(identity); + return true; + }); +} + +const RESTORE_PENDING_COMMENTS_SCRIPT = ` +local existing = redis.call('LRANGE', KEYS[1], 0, -1) +local seen = {} +for _, raw in ipairs(existing) do + local ok, value = pcall(cjson.decode, raw) + if ok and value then + local commentType = value.type or 'issue' + local revision = value.updatedAt or value.createdAt or '' + local legacyIdentity = commentType .. ':' .. tostring(value.id) .. ':' .. revision .. ':' .. (value.body or '') + seen[legacyIdentity] = true + if value.revisionIdentity then + seen[commentType .. ':' .. tostring(value.id) .. ':' .. value.revisionIdentity] = true + end + end +end + +local missing = {} +for index = 1, #ARGV, 3 do + local identity = ARGV[index] + local legacyIdentity = ARGV[index + 1] + if not seen[identity] and not seen[legacyIdentity] then + table.insert(missing, ARGV[index + 2]) + seen[identity] = true + seen[legacyIdentity] = true + end +end + +for index = #missing, 1, -1 do + redis.call('LPUSH', KEYS[1], missing[index]) +end +if #missing > 0 then redis.call('EXPIRE', KEYS[1], 3600) end +return #missing +`; + +/** + * Atomically restore comments to the head of the pending list without adding a + * second copy during retry/redelivery. Existing pending arrivals retain order. + */ +export async function restorePendingCommentsIdempotently( + redisClient: Redis, + pendingCommentsKey: string, + comments: UnprocessedComment[], +): Promise { + const uniqueComments = dedupeUnprocessedComments(comments); + if (uniqueComments.length === 0) return 0; + const args = uniqueComments.flatMap(comment => { + const revisionIdentity = getUnprocessedCommentRevisionIdentity(comment); + const revision = comment.updatedAt ?? comment.createdAt ?? ''; + return [ + `${comment.type}:${comment.id}:${revisionIdentity}`, + `${comment.type}:${comment.id}:${revision}:${comment.body}`, + JSON.stringify({ ...comment, revisionIdentity }), + ]; + }); + return Number(await redisClient.eval(RESTORE_PENDING_COMMENTS_SCRIPT, 1, pendingCommentsKey, ...args)); +} diff --git a/packages/core/src/utils/ultrafixLabelTransition.ts b/packages/core/src/utils/ultrafixLabelTransition.ts index 99c1847cc..040bfac40 100644 --- a/packages/core/src/utils/ultrafixLabelTransition.ts +++ b/packages/core/src/utils/ultrafixLabelTransition.ts @@ -26,15 +26,28 @@ export interface LabelTransitionTiming { waitMs: number; } +export interface LabelTransitionLease { + identity: Readonly<{ owner: string; repo: string; pr: number }>; + /** Verify and renew ownership before an irreversible transition step. */ + assertOwned: () => Promise; +} + +export class LabelTransitionLeaseError extends Error { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = 'LabelTransitionLeaseError'; + } +} + function getLabelTransitionLockKey(identity: { owner: string; repo: string; pr: number }): string { return `${LABEL_TRANSITION_LOCK_KEY_PREFIX}:${identity.owner}:${identity.repo}:${identity.pr}`; } -/** Serialize the shared GitHub label with epoch state publication and cleanup. */ -export async function withUltrafixLabelTransition( +/** Serialize all label-transition work for one owner/repository/PR. */ +export async function withLabelTransitionLease( redis: Pick, identity: { owner: string; repo: string; pr: number }, - operation: () => Promise, + operation: (lease: LabelTransitionLease) => Promise, timing: LabelTransitionTiming = { ttlMs: LABEL_TRANSITION_LOCK_TTL_MS, renewIntervalMs: LABEL_TRANSITION_RENEW_INTERVAL_MS, @@ -44,8 +57,16 @@ export async function withUltrafixLabelTransition( const key = getLabelTransitionLockKey(identity); const token = randomUUID(); const deadline = Date.now() + timing.waitMs; - while (await redis.set(key, token, 'PX', timing.ttlMs, 'NX') !== 'OK') { - if (Date.now() >= deadline) throw new Error('Timed out waiting for Ultrafix label transition'); + let acquired = false; + while (!acquired) { + try { + acquired = await redis.set(key, token, 'PX', timing.ttlMs, 'NX') === 'OK'; + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new LabelTransitionLeaseError(`Failed to acquire PR label transition lease: ${detail}`, { cause: error }); + } + if (acquired) break; + if (Date.now() >= deadline) throw new LabelTransitionLeaseError('Timed out waiting for PR label transition lease'); await new Promise(resolve => setTimeout(resolve, 100)); } let renewal: Promise | null = null; @@ -68,21 +89,30 @@ export async function withUltrafixLabelTransition( }; const renewalTimer = setInterval(renew, timing.renewIntervalMs); renewalTimer.unref(); + const assertOwned = async (): Promise => { + await renewal; + if (!leaseLost) { + try { + const stillOwned = await redis.eval( + RENEW_LABEL_TRANSITION_LOCK_SCRIPT, + 1, + key, + token, + String(timing.ttlMs), + ); + if (Number(stillOwned) !== 1) leaseLost = true; + } catch { + leaseLost = true; + } + } + if (leaseLost) throw new LabelTransitionLeaseError('PR label transition lease was lost'); + }; try { - const result = await operation(); + const result = await operation({ identity, assertOwned }); // Stop new heartbeats, wait for one already in flight, then verify that // this transition still owns the lease before publishing success. clearInterval(renewalTimer); - await renewal; - const stillOwned = await redis.eval( - RENEW_LABEL_TRANSITION_LOCK_SCRIPT, - 1, - key, - token, - String(timing.ttlMs), - ); - if (Number(stillOwned) !== 1) leaseLost = true; - if (leaseLost) throw new Error('Ultrafix label transition lease was lost'); + await assertOwned(); return result; } finally { clearInterval(renewalTimer); @@ -97,6 +127,9 @@ export async function withUltrafixLabelTransition( } } +/** Serialize the shared GitHub label with epoch state publication and cleanup. */ +export const withUltrafixLabelTransition = withLabelTransitionLease; + export type UltrafixLabelRemovalResult = 'cleared' | 'label_present' | 'unverified'; /** Clear loop state only when the live label is still absent under the transition lease. */ diff --git a/packages/core/src/webhook/commentEventHandler.ts b/packages/core/src/webhook/commentEventHandler.ts index f4a7560e6..6d6666346 100644 --- a/packages/core/src/webhook/commentEventHandler.ts +++ b/packages/core/src/webhook/commentEventHandler.ts @@ -6,11 +6,16 @@ import { filterCommentByAuthor, checkCommentTrigger, checkCommentIgnore } from ' import { loadFollowupIgnoreKeywords, loadPrimaryProcessingLabels } from '../config/configManager.js'; import { getAuthenticatedOctokit } from '../auth/githubAuth.js'; import { getPendingPrCommentsKey } from '../utils/constants.js'; +import { getUnprocessedCommentRevisionIdentity, restorePendingCommentsIdempotently } from '../utils/pendingComments.js'; import { withRetry } from '../utils/retryHandler.js'; import type { Job } from 'bullmq'; import type { Redis } from 'ioredis'; -import { createHash } from 'node:crypto'; -import { withUltrafixLabelTransition } from '../utils/ultrafixLabelTransition.js'; +import { + LabelTransitionLeaseError, + withLabelTransitionLease, + withUltrafixLabelTransition, + type LabelTransitionLease, +} from '../utils/ultrafixLabelTransition.js'; import type { IssueCommentEvent, PullRequestReviewCommentEvent, Label } from '@octokit/webhooks-types'; import { extractLlmFromKeywords, stripKeywordsFromBody, buildCodeContext, isReviewComment, extractLlmFromLabels, modelLabelPrefix } from './commentEventHelpers.js'; import { handleMergeCommand } from './mergeConflictDetector.js'; @@ -18,9 +23,8 @@ import { parseSlashCommand, buildCommandMeta } from './slashCommandParser.js'; import type { CommandMeta, UltrafixCommandMeta } from './slashCommandParser.js'; import { safeUpdateLabels } from '../utils/github/labelOperations.js'; import { resolveModelAlias } from '../config/modelAliases.js'; -import { MODEL_INFO_MAP } from '../config/modelDefinitions.js'; +import { getAllCustomLabels, resolveCanonicalModelSelection, type CanonicalModelSelection } from '../config/modelLabelResolution.js'; import { getBotUsername } from '../daemon/configLoader.js'; -import { AgentRegistry } from '../agents/AgentRegistry.js'; import type { DeliveryDisposition } from '../intake/routingWebSocketProtocol.js'; export interface UltrafixDeps { @@ -49,24 +53,6 @@ function loadUltrafixDeps(): UltrafixDeps { return _ultrafixDeps; } -async function isKnownOrConfiguredModel(model: string): Promise { - if (MODEL_INFO_MAP[model]) { - return true; - } - - try { - const registry = AgentRegistry.getInstance(); - await registry.ensureInitialized(); - return registry.getAllAgents().some(agent => - agent.config.enabled && agent.config.supportedModels.some( - supportedModel => supportedModel.toLowerCase() === model.toLowerCase() - ) - ); - } catch { - return false; - } -} - export type CommentEventType = 'issue_comment' | 'pull_request_review_comment'; export interface CommentEventConfig { @@ -87,17 +73,20 @@ interface PRJobData extends CommentJobData { interface CommentContext { eventType: CommentEventType; prNumber: number; owner: string; repo: string } interface StoreCommentConfig { redisClient: Redis; PR_FOLLOWUP_TRIGGER_KEYWORDS: string[] } -interface EnqueueCommentOptions { payload: IssueCommentEvent | PullRequestReviewCommentEvent; redisClient: Redis; PR_FOLLOWUP_TRIGGER_KEYWORDS: string[]; MODEL_LABEL_PATTERN?: string; correlationId: string; commandMeta?: CommandMeta; prefetchedPRData?: PRBranchAndLabels; ultrafixMeta?: UltrafixCommandMeta; commentRevisionIdentity?: string } +interface EnqueueCommentOptions { payload: IssueCommentEvent | PullRequestReviewCommentEvent; redisClient: Redis; PR_FOLLOWUP_TRIGGER_KEYWORDS: string[]; MODEL_LABEL_PATTERN?: string; correlationId: string; commandMeta?: CommandMeta; prefetchedPRData?: PRBranchAndLabels; ultrafixMeta?: UltrafixCommandMeta; commentRevisionIdentity?: string; modelSelection?: CanonicalModelSelection; pendingOnly?: boolean } interface RepoContext { owner: string; repo: string; prNumber: number } interface PRBranchAndLabels { branchName: string; prLabels: Label[] } -type BatchComment = Pick & { created_at: string; path?: string; line?: number | null; diff_hunk?: string; pull_request_review_id?: number }; +type BatchComment = Pick & { created_at: string; updated_at: string; path?: string; line?: number | null; diff_hunk?: string; pull_request_review_id?: number }; type CommandJobFields = Pick; type PRComment = { id: number; created_at: string; updated_at: string; body: string; user: { login: string; type?: string }; path?: string; line?: number | null; diff_hunk?: string; pull_request_review_id?: number }; type ManualCommandTakeover = { workEpoch: number; hadAutomaticWork: boolean; commentRevisionIdentity: string }; function getCommentRevisionIdentity(comment: Pick, eventType: CommentEventType): string { - const contentDigest = createHash('sha256').update(`${eventType}\0${comment.body}`).digest('hex').slice(0, 12); - return `${comment.updated_at}:${contentDigest}`; + return getUnprocessedCommentRevisionIdentity({ + updatedAt: comment.updated_at, + body: comment.body, + type: eventType === 'pull_request_review_comment' ? 'review' : 'issue', + }); } async function claimCommentForProcessing(redisClient: Redis, key: string): Promise { @@ -235,7 +224,7 @@ type ManualCommandFenceOptions = Pick { const { commandMeta, comment, eventContext, config, correlatedLogger } = opts; - if (commandMeta.mode !== 'fix' && commandMeta.mode !== 'review') return null; + if (commandMeta.mode !== 'fix' && commandMeta.mode !== 'review' && commandMeta.mode !== 'use') return null; const { owner, repo, prNumber } = eventContext; const commentRevisionIdentity = getCommentRevisionIdentity(comment, eventContext.eventType); @@ -278,30 +267,19 @@ async function handleSlashCommand(opts: SlashCommandHandlerOptions): Promise 0) { - const resolvedModel = resolveModelAlias(commandMeta.models[0]); - if (!await isKnownOrConfiguredModel(resolvedModel)) { - correlatedLogger.warn({ pullRequestNumber: prNumber, invalidModels: [resolvedModel] }, '/use command contains unrecognized model(s), ignoring'); - return; - } - } - const manualTakeover = await fenceManualCommand({ commandMeta, comment, eventContext, config, correlatedLogger }); + const commentRevisionIdentity = manualTakeover?.commentRevisionIdentity + ?? getCommentRevisionIdentity(comment, eventContext.eventType); correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, commentAuthor, command: commandMeta.mode }, `/${commandMeta.mode} command detected, enqueuing job`); // Strip the slash command line from the comment body so the downstream job // only sees the user's instructions, not the control syntax (consistent with /switch). - const strippedComment = { ...comment, body: commandMeta.instructions || '' }; + const strippedComment = { ...comment, body: commandMeta.instructions || '', revisionIdentity: commentRevisionIdentity }; // Check for existing active/waiting jobs for this PR (batching/concurrency guard). // The Redis loop state may already be inactive while its BullMQ job is still @@ -322,84 +300,426 @@ async function handleSlashCommand(opts: SlashCommandHandlerOptions): Promise & { commandMeta: CommandMeta & { mode: 'switch' } }; +type ModelSelectionCommandOptions = Omit & { commandMeta: CommandMeta & { mode: 'switch' | 'use' } }; -async function handleSwitchCommand(opts: SwitchCommandOptions): Promise { - const { commandMeta, comment, commentAuthor, eventContext, payload, config, correlationId, correlatedLogger } = opts; - const { eventType, prNumber, owner, repo } = eventContext; - const { redisClient } = config; +interface PRCommentJobSnapshot { + active: Job[]; + waiting: Job[]; + delayed: Job[]; +} - if (commandMeta.models.length === 0) { - correlatedLogger.warn({ pullRequestNumber: prNumber, commentId: comment.id, commentAuthor }, '/switch command requires a model argument, ignoring'); - return; +interface CommandChronology { + id: number; + createdAt?: string; + updatedAt?: string; + revisionIdentity?: string; + type?: UnprocessedComment['type']; + ingestionOrder?: number; +} + +function compareOptionalStrings(left: string | undefined, right: string | undefined): number { + if (left === right) return 0; + if (left === undefined) return -1; + if (right === undefined) return 1; + return left.localeCompare(right); +} + +function compareCommandTypes(left: CommandChronology['type'], right: CommandChronology['type']): number { + if (left === right) return 0; + if (left === undefined) return -1; + if (right === undefined) return 1; + return left === 'issue' ? -1 : 1; +} + +function compareCommandChronology(left: CommandChronology, right: CommandChronology, useCreatedAt: boolean): number { + if (left.id === right.id && left.type === right.type) { + const revisionOrder = compareOptionalStrings(left.updatedAt ?? left.createdAt, right.updatedAt ?? right.createdAt); + if (revisionOrder !== 0) return revisionOrder; + if (left.ingestionOrder !== right.ingestionOrder) return (left.ingestionOrder ?? -1) - (right.ingestionOrder ?? -1); + return compareOptionalStrings(left.revisionIdentity, right.revisionIdentity); } - const resolvedModels = commandMeta.models.map(m => resolveModelAlias(m)); - const invalidModels: string[] = []; - for (const model of resolvedModels) { - if (!await isKnownOrConfiguredModel(model)) { - invalidModels.push(model); - } + if (useCreatedAt && left.createdAt !== undefined && right.createdAt !== undefined) { + const createdAtOrder = left.createdAt.localeCompare(right.createdAt); + if (createdAtOrder !== 0) return createdAtOrder; + const typeOrder = compareCommandTypes(left.type, right.type); + if (typeOrder !== 0) return typeOrder; } - if (invalidModels.length > 0) { - correlatedLogger.warn({ pullRequestNumber: prNumber, invalidModels }, '/switch command contains unrecognized model(s), ignoring'); - return; + + const idOrder = left.id - right.id; + if (idOrder !== 0 || useCreatedAt) return idOrder; + return compareCommandTypes(left.type, right.type); +} + +function getJobCommandChronologies(job: Job): CommandChronology[] { + const data = job.data; + if (!data.commandMode || data.commandMode === 'default') return []; + if (data.commandCommentId === undefined) { + return data.comments ?? (data.commentId === undefined ? [] : [{ id: data.commentId }]); } - correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, commentAuthor, models: commandMeta.models }, '/switch command detected, updating PR labels'); - const prData = await getPRBranchAndLabels(eventType, payload, { owner, repo, prNumber }); - const { prLabels } = prData; - const modelLabelPattern = config.MODEL_LABEL_PATTERN || '^llm-(.+)$'; + const candidates = (data.comments ?? []).map((comment, ingestionOrder) => ({ + ...comment, + revisionIdentity: getUnprocessedCommentRevisionIdentity(comment), + ingestionOrder, + })).filter(comment => comment.id === data.commandCommentId + && (data.commandCommentType === undefined || comment.type === data.commandCommentType)); + const ownerComment = candidates.find(comment => data.commandCommentRevisionIdentity !== undefined + && comment.revisionIdentity === data.commandCommentRevisionIdentity) + ?? candidates.find(comment => data.commandCommentUpdatedAt !== undefined + && comment.updatedAt === data.commandCommentUpdatedAt) + ?? candidates[0]; + return [{ + id: data.commandCommentId, + createdAt: data.commandCommentCreatedAt ?? ownerComment?.createdAt, + updatedAt: data.commandCommentUpdatedAt ?? ownerComment?.updatedAt, + revisionIdentity: data.commandCommentRevisionIdentity ?? ownerComment?.revisionIdentity, + type: data.commandCommentType ?? ownerComment?.type, + }]; +} + +function snapshotContainsCommandRevision(snapshot: PRCommentJobSnapshot, incoming: CommandChronology): boolean { + return [...snapshot.active, ...snapshot.waiting, ...snapshot.delayed] + .flatMap(getJobCommandChronologies) + .some(existing => existing.id === incoming.id + && existing.type === incoming.type + && existing.revisionIdentity === incoming.revisionIdentity); +} + +function latestPendingCommandChronologies(comments: UnprocessedComment[]): CommandChronology[] { + const latestByComment = new Map(); + comments.forEach((comment, ingestionOrder) => { + if ((!comment.commandMode || comment.commandMode === 'default') && comment.llmOverride === undefined) return; + const chronology = { + ...comment, + revisionIdentity: getUnprocessedCommentRevisionIdentity(comment), + ingestionOrder, + }; + const key = `${comment.type}:${comment.id}`; + const latest = latestByComment.get(key); + if (!latest || compareCommandChronology(chronology, latest, true) > 0) latestByComment.set(key, chronology); + }); + return [...latestByComment.values()]; +} + +async function findNewerQueuedCommand( + incoming: CommandChronology, + eventContext: CommentContext, + snapshot: PRCommentJobSnapshot, + redisClient: Redis, +): Promise { + const pendingKey = getPendingPrCommentsKey(eventContext.owner, eventContext.repo, eventContext.prNumber); + const pendingComments = (await redisClient.lrange(pendingKey, 0, -1)).map(raw => JSON.parse(raw) as UnprocessedComment); + const existing = [ + ...snapshot.active, + ...snapshot.waiting, + ...snapshot.delayed, + ].flatMap(getJobCommandChronologies).concat(latestPendingCommandChronologies(pendingComments)); + const orderedExisting = existing.map((record, ingestionOrder) => ({ ...record, ingestionOrder })); + const useCreatedAt = [...orderedExisting, incoming].every(record => record.createdAt !== undefined); + const latest = orderedExisting.reduce((current, record) => + !current || compareCommandChronology(record, current, useCreatedAt) > 0 ? record : current, undefined); + return latest && compareCommandChronology(incoming, latest, useCreatedAt) < 0 ? latest : undefined; +} + +function getModelCommandSequenceKey(eventContext: CommentContext): string { + return `pr-model-command-sequence:${eventContext.owner}:${eventContext.repo}:${eventContext.prNumber}`; +} + +function getModelCommandRevisionKey(eventContext: CommentContext): string { + return `pr-model-command-revision:${eventContext.owner}:${eventContext.repo}:${eventContext.prNumber}`; +} + +async function nextModelCommandIngestionOrder(redisClient: Redis, eventContext: CommentContext): Promise { + const sequenceKey = getModelCommandSequenceKey(eventContext); + const ingestionOrder = await redisClient.incr(sequenceKey); + await redisClient.expire(sequenceKey, 86400); + return ingestionOrder; +} + +async function claimLatestModelCommand( + redisClient: Redis, + eventContext: CommentContext, + incoming: CommandChronology, +): Promise { + const markerKey = getModelCommandRevisionKey(eventContext); + const rawMarker = await redisClient.get(markerKey); + if (rawMarker) { + const current = JSON.parse(rawMarker) as CommandChronology; + const useCreatedAt = current.createdAt !== undefined && incoming.createdAt !== undefined; + if (compareCommandChronology(incoming, current, useCreatedAt) < 0) return false; + } + await redisClient.set(markerKey, JSON.stringify(incoming), 'EX', 86400); + return true; +} + +async function resolveCompatibleModelSelection( + selection: CanonicalModelSelection, + modelLabelPattern: string, +): Promise { const modelLabelRegex = new RegExp(modelLabelPattern); + const configuredCustomLabels = new Set((await getAllCustomLabels()).map(label => label.toLowerCase())); + if (modelLabelRegex.test(selection.githubLabel) || configuredCustomLabels.has(selection.githubLabel.toLowerCase())) { + return selection; + } - const existingLlmLabels = prLabels.filter(l => modelLabelRegex.test(l.name)).map(l => l.name); const { prefix, derived } = modelLabelPrefix(modelLabelPattern); - if (!derived) { - correlatedLogger.warn({ pullRequestNumber: prNumber, modelLabelPattern }, 'Could not derive label prefix from MODEL_LABEL_PATTERN, falling back to default "llm-". Labels may be mismatched.'); - } - const newLabels = resolvedModels.map(m => `${prefix}${m}`); + const canonicalSuffix = selection.githubLabel.match(/^llm-(.+)$/i)?.[1]; + if (!derived || !canonicalSuffix) return null; + + const candidate = `${prefix}${canonicalSuffix}`; + const match = candidate.match(modelLabelRegex); + if (!match || match.length !== 2 || !match[1]) return null; + const roundTrip = await resolveCanonicalModelSelection(match[1]); + if (!roundTrip + || roundTrip.agentAlias.toLowerCase() !== selection.agentAlias.toLowerCase() + || roundTrip.model.toLowerCase() !== selection.model.toLowerCase()) return null; + return { ...selection, githubLabel: candidate }; +} - // Validate that newly constructed labels match the configured regex. - // If they don't, a future /switch would fail to detect them as existing - // model labels, causing duplicates instead of replacements. - const mismatchedLabels = newLabels.filter(l => !modelLabelRegex.test(l)); - if (mismatchedLabels.length > 0) { - correlatedLogger.error({ pullRequestNumber: prNumber, mismatchedLabels, modelLabelPattern, derivedPrefix: prefix }, '/switch: derived label prefix produces labels that do not match MODEL_LABEL_PATTERN — aborting to prevent label duplication'); - return; +async function postModelSelectionAcknowledgement( + opts: Pick, + selection: CanonicalModelSelection, + outcome: 'label-only' | 'queued' | 'pending', +): Promise { + const { owner, repo, prNumber } = opts.eventContext; + const suffix = outcome === 'pending' + ? ' The follow-up is saved for the active writer.' + : outcome === 'queued' + ? ' The follow-up has been queued.' + : ''; + try { + const octokit = await getAuthenticatedOctokit(); + await octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { + owner, + repo, + issue_number: prNumber, + body: `✅ Model switched to \`${selection.githubLabel}\` (\`${selection.agentAlias}:${selection.model}\`).${suffix}`, + }); + } catch (error) { + opts.correlatedLogger.warn({ pullRequestNumber: prNumber, error: (error as Error).message }, 'Model switch succeeded but acknowledgement could not be posted'); } +} + +async function transitionModelLabel( + opts: ModelSelectionCommandOptions, + prLabels: Label[], + selection: CanonicalModelSelection, + lease: LabelTransitionLease, +): Promise<{ success: boolean; updatedLabels: Label[] }> { + const { eventContext: { owner, repo, prNumber }, config: { redisClient }, config, correlatedLogger } = opts; + const modelLabelPattern = config.MODEL_LABEL_PATTERN || '^llm-(.+)$'; + const modelLabelRegex = new RegExp(modelLabelPattern); + const configuredCustomLabels = new Set((await getAllCustomLabels()).map(label => label.toLowerCase())); + const existingModelLabels = prLabels + .filter(label => modelLabelRegex.test(label.name) || configuredCustomLabels.has(label.name.toLowerCase())) + .map(label => label.name); + const targetAlreadyPresent = existingModelLabels.some(label => label.toLowerCase() === selection.githubLabel.toLowerCase()); + const labelsToRemove = existingModelLabels.filter(label => label.toLowerCase() !== selection.githubLabel.toLowerCase()); + const labelsToAdd = targetAlreadyPresent ? [] : [selection.githubLabel]; const octokit = await getAuthenticatedOctokit(); - await safeUpdateLabels( + const result = await safeUpdateLabels( { octokit, owner, repo, issueNumber: prNumber, logger: correlatedLogger }, - existingLlmLabels, - newLabels + labelsToRemove, + labelsToAdd, + { + targetLabel: selection.githubLabel, + isManagedLabel: labelName => + modelLabelRegex.test(labelName) + || configuredCustomLabels.has(labelName.toLowerCase()), + maxAttempts: 3, + redis: redisClient, + lease, + }, ); + if (!result.success) { + correlatedLogger.error({ pullRequestNumber: prNumber, targetLabel: selection.githubLabel, errors: result.errors }, 'Model label transition failed; follow-up will not be queued'); + return { success: false, updatedLabels: prLabels }; + } + + const verifiedLabelNames = result.finalLabels ?? [ + ...prLabels.filter(label => !labelsToRemove.includes(label.name)).map(label => label.name), + ...(!targetAlreadyPresent ? [selection.githubLabel] : []), + ]; + const updatedLabels = verifiedLabelNames.map(name => ({ + id: 0, name, node_id: '', url: '', color: '', default: false, description: null, + } as Label)); + return { success: true, updatedLabels }; +} + +function isProviderLimitRetry(job: Job): boolean { + return job.data.isRetryFromRateLimit === true || String(job.id ?? '').endsWith('-ratelimit-retry'); +} + +async function restoreSupersededRetryComments(jobs: Job[], eventContext: CommentContext, redisClient: Redis): Promise { + const comments = jobs.flatMap(job => job.data.comments ?? []); + const pendingCommentsKey = getPendingPrCommentsKey(eventContext.owner, eventContext.repo, eventContext.prNumber); + await restorePendingCommentsIdempotently(redisClient, pendingCommentsKey, comments); +} + +async function supersedeProviderLimitRetries(snapshot: PRCommentJobSnapshot, eventContext: CommentContext, redisClient: Redis): Promise { + const retryJobs = [...snapshot.waiting, ...snapshot.delayed].filter(isProviderLimitRetry); + if (retryJobs.length === 0) return 0; + await restoreSupersededRetryComments(retryJobs, eventContext, redisClient); + for (const job of retryJobs) await job.remove(); + return retryJobs.length; +} + +async function handleModelSelectionCommand(opts: ModelSelectionCommandOptions): Promise { + const { commandMeta, comment, commentAuthor, eventContext, payload, config, correlationId, correlatedLogger } = opts; + const { eventType, prNumber, owner, repo } = eventContext; + const { redisClient } = config; + const commandName = commandMeta.mode; - if (!commandMeta.instructions) { - correlatedLogger.info({ pullRequestNumber: prNumber }, '/switch command has no instructions, label update complete'); + if (commandMeta.models.length === 0) { + correlatedLogger.warn({ pullRequestNumber: prNumber, commentId: comment.id, commentAuthor }, `/${commandName} command requires a model argument, ignoring`); return; } + const ingestionOrder = await nextModelCommandIngestionOrder(redisClient, eventContext); - correlatedLogger.info({ pullRequestNumber: prNumber }, '/switch command has instructions, enqueuing follow-up job'); - // Strip the /switch command line from the comment body so the downstream job - // only sees the user's instructions, not the control syntax. - const strippedComment = { ...comment, body: commandMeta.instructions }; + const canonicalSelection = await resolveCanonicalModelSelection(commandMeta.models[0]); + if (!canonicalSelection) { + correlatedLogger.warn({ pullRequestNumber: prNumber, requestedModel: commandMeta.models[0] }, `/${commandName} command contains an unrecognized model or an unconfigured model, ignoring`); + return; + } + const modelLabelPattern = config.MODEL_LABEL_PATTERN || '^llm-(.+)$'; + const selection = await resolveCompatibleModelSelection(canonicalSelection, modelLabelPattern); + if (!selection) { + correlatedLogger.error({ pullRequestNumber: prNumber, targetLabel: canonicalSelection.githubLabel, modelLabelPattern }, 'Configured model label pattern cannot represent the selected model unambiguously'); + return; + } + const selectedCommandMeta = { ...commandMeta, models: [selection.model] } as CommandMeta & { mode: 'switch' | 'use' }; + const commentRevisionIdentity = getCommentRevisionIdentity(comment, eventType); + const commandChronology: CommandChronology = { + id: comment.id, + createdAt: comment.created_at, + updatedAt: comment.updated_at, + revisionIdentity: commentRevisionIdentity, + type: eventType === 'pull_request_review_comment' ? 'review' : 'issue', + ingestionOrder, + }; - // Check for existing active/waiting jobs for this PR (batching/concurrency guard) - const existingSwitchJob = await checkExistingJob(prNumber, owner, repo); - if (existingSwitchJob) { - await storeCommentForBatch({ ...strippedComment, ...buildPendingCommandFields(commandMeta) }, commentAuthor, eventContext, { redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS }); - correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id }, '/switch command: existing job found for PR, stored follow-up instructions for batch processing'); + const chronologySnapshot = await getPRCommentJobSnapshot(prNumber, owner, repo); + const newerCommand = await findNewerQueuedCommand(commandChronology, eventContext, chronologySnapshot, redisClient); + if (newerCommand) { + correlatedLogger.info({ + pullRequestNumber: prNumber, + staleCommentId: comment.id, + staleCommentUpdatedAt: comment.updated_at, + newerCommentId: newerCommand.id, + newerCommentUpdatedAt: newerCommand.updatedAt, + }, `Ignoring stale /${commandName} delivery before model label transition`); return; } - // Re-use already-fetched PR data to avoid a redundant GitHub API call. - // The labels have been updated above, so reflect the new labels in the prefetched data. - const updatedPRData = { branchName: prData.branchName, prLabels: [...prLabels.filter(l => !existingLlmLabels.includes(l.name)), ...newLabels.map(n => ({ id: 0, name: n, node_id: '', url: '', color: '', default: false, description: null }))] as Label[] }; - await enqueueNewCommentJob(strippedComment, commentAuthor, eventContext, { payload, redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS, MODEL_LABEL_PATTERN: config.MODEL_LABEL_PATTERN, correlationId, commandMeta, prefetchedPRData: updatedPRData }); + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, commentAuthor, selection }, `/${commandName} command detected, updating PR model label`); + const prData = await getPRBranchAndLabels(eventType, payload, { owner, repo, prNumber }); + let acknowledgementOutcome: 'label-only' | 'queued' | 'pending' | undefined; + try { + await withLabelTransitionLease(redisClient, { owner, repo, pr: prNumber }, async lease => { + if (!await claimLatestModelCommand(redisClient, eventContext, commandChronology)) { + correlatedLogger.info({ pullRequestNumber: prNumber, staleCommentId: commandChronology.id }, 'Ignoring stale model command after acquiring the label transition lease'); + return; + } + + const transition = await transitionModelLabel(opts, prData.prLabels, selection, lease); + if (!transition.success) return; + + const shouldQueue = commandMeta.mode === 'use' || Boolean(commandMeta.instructions); + if (!shouldQueue) { + correlatedLogger.info({ pullRequestNumber: prNumber, selection }, '/switch command has no instructions, durable model switch complete'); + acknowledgementOutcome = 'label-only'; + return; + } + + if (snapshotContainsCommandRevision(chronologySnapshot, commandChronology)) { + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, selection }, `/${commandName} command revision was already durably queued before retry`); + acknowledgementOutcome = 'queued'; + return; + } + + // /use is a manual takeover just like /fix and /review. Keep the + // fence and every queue-or-pending handoff under the same PR lease + // as the freshness claim and verified label convergence. + await lease.assertOwned(); + const manualTakeover = await fenceManualCommand({ commandMeta, comment, eventContext, config, correlatedLogger }); + const strippedComment = { ...comment, body: commandMeta.instructions || '', revisionIdentity: commentRevisionIdentity }; + const snapshot = await getPRCommentJobSnapshot(prNumber, owner, repo); + + if (snapshot.active.length > 0) { + await lease.assertOwned(); + await storeCommentForBatch({ ...strippedComment, ...buildPendingCommandFields(selectedCommandMeta, selection) }, commentAuthor, eventContext, { redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS }); + if (commandMeta.mode !== 'use') { + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, selection }, `/${commandName} command: active writer found, stored selected follow-up for the next run`); + acknowledgementOutcome = 'pending'; + return; + } + // The active worker may already have crossed its final pending-comment + // check while BullMQ still reports it as active. Always enqueue a + // deterministic pending-only successor after the write so one of the + // two jobs is guaranteed to claim this revision. + const updatedPRData = { branchName: prData.branchName, prLabels: transition.updatedLabels }; + await lease.assertOwned(); + await enqueueNewCommentJob(strippedComment, commentAuthor, eventContext, { + payload, redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS, + MODEL_LABEL_PATTERN: config.MODEL_LABEL_PATTERN, correlationId, + commandMeta: selectedCommandMeta, prefetchedPRData: updatedPRData, modelSelection: selection, + commentRevisionIdentity, pendingOnly: true, + }); + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, selection }, `/${commandName} command: active writer found, stored selected follow-up and queued its successor`); + acknowledgementOutcome = 'queued'; + return; + } + + const hasQueuedProviderRetries = [...snapshot.waiting, ...snapshot.delayed].some(isProviderLimitRetry); + if (hasQueuedProviderRetries) { + // Make the selected follow-up recoverable before removing its previous + // owner. It is identity-deduplicated when the replacement claims it. + await lease.assertOwned(); + await storeCommentForBatch( + { ...strippedComment, ...buildPendingCommandFields(selectedCommandMeta, selection) }, + commentAuthor, + eventContext, + { redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS }, + ); + } + await lease.assertOwned(); + const supersededRetries = await supersedeProviderLimitRetries(snapshot, eventContext, redisClient); + const remainingQueuedJobs = [...snapshot.waiting, ...snapshot.delayed].filter(job => !isProviderLimitRetry(job)); + const requiresIndependentTakeover = shouldEnqueueIndependentManualTakeover(remainingQueuedJobs, manualTakeover); + if (remainingQueuedJobs.length > 0 && !requiresIndependentTakeover) { + await lease.assertOwned(); + await storeCommentForBatch({ ...strippedComment, ...buildPendingCommandFields(selectedCommandMeta, selection) }, commentAuthor, eventContext, { redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS }); + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, selection }, `/${commandName} command: queued writer found, stored selected follow-up for batching`); + acknowledgementOutcome = 'pending'; + return; + } + if (remainingQueuedJobs.length > 0) { + correlatedLogger.info({ pullRequestNumber: prNumber, commentId: comment.id, selection }, `/${commandName} command: fenced automatic work will be replaced by an independent durable job`); + } + + const updatedPRData = { branchName: prData.branchName, prLabels: transition.updatedLabels }; + await lease.assertOwned(); + await enqueueNewCommentJob(strippedComment, commentAuthor, eventContext, { + payload, redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS: config.PR_FOLLOWUP_TRIGGER_KEYWORDS, + MODEL_LABEL_PATTERN: config.MODEL_LABEL_PATTERN, correlationId, + commandMeta: selectedCommandMeta, prefetchedPRData: updatedPRData, modelSelection: selection, + commentRevisionIdentity, + }); + correlatedLogger.info({ pullRequestNumber: prNumber, supersededRetries, selection }, `/${commandName} follow-up queued with durable model selection`); + acknowledgementOutcome = 'queued'; + }); + } catch (error) { + if (!(error instanceof LabelTransitionLeaseError)) throw error; + correlatedLogger.warn({ error: error.message, pullRequestNumber: prNumber }, 'Model command failed to hold its complete PR transition lease'); + throw error; + } + + if (acknowledgementOutcome) await postModelSelectionAcknowledgement(opts, selection, acknowledgementOutcome); } type UltrafixCommandOptions = Omit & { commandMeta: UltrafixCommandMeta }; @@ -429,7 +749,8 @@ async function handleUltrafixCommand(opts: UltrafixCommandOptions): Promise[]> { + const snapshot = await getPRCommentJobSnapshot(prNumber, owner, repo); + return [...snapshot.active, ...snapshot.waiting, ...snapshot.delayed]; +} + +async function getPRCommentJobSnapshot(prNumber: number, owner: string, repo: string): Promise { const queue = await getIssueQueue(); const [activeJobs, waitingJobs, delayedJobs] = await Promise.all([ queue.getActive(), queue.getWaiting(), queue.getDelayed(), ]); - const existingJobs = [...activeJobs, ...waitingJobs, ...delayedJobs] as Job[]; - return existingJobs.filter(job => job.name === 'processPullRequestComment' && job.data.pullRequestNumber === prNumber && job.data.repoOwner === owner && job.data.repoName === repo); + const belongsToPR = (job: Job): boolean => job.name === 'processPullRequestComment' + && job.data.pullRequestNumber === prNumber + && job.data.repoOwner === owner + && job.data.repoName === repo; + return { + active: (activeJobs as Job[]).filter(belongsToPR), + waiting: (waitingJobs as Job[]).filter(belongsToPR), + delayed: (delayedJobs as Job[]).filter(belongsToPR), + }; } function shouldEnqueueIndependentManualTakeover( @@ -719,6 +1053,12 @@ async function storeCommentForBatch(comment: BatchComment, commentAuthor: string const pendingComment: UnprocessedComment = { id: comment.id, createdAt: comment.created_at, + updatedAt: comment.updated_at, + revisionIdentity: comment.revisionIdentity ?? getUnprocessedCommentRevisionIdentity({ + updatedAt: comment.updated_at, + body: comment.body, + type: reviewComment ? 'review' : 'issue', + }), body: pendingCommentBody, author: commentAuthor, type: reviewComment ? 'review' : 'issue', @@ -728,6 +1068,9 @@ async function storeCommentForBatch(comment: BatchComment, commentAuthor: string requestedModels: comment.requestedModels, commandInstructions: comment.commandInstructions, llmOverride: comment.llmOverride, + agentAlias: comment.agentAlias, + modelName: comment.modelName, + modelLabel: comment.modelLabel, ultrafixMeta: comment.ultrafixMeta, }; await redisClient.rpush(pendingCommentsKey, JSON.stringify(pendingComment)); @@ -754,7 +1097,13 @@ async function getLivePRBranchAndLabels(repoContext: RepoContext): Promise 0 ? extractLlmFromKeywords(comment.body, keywords) : null; let enhancedBody = keywords.length > 0 ? stripKeywordsFromBody(comment.body, keywords) : comment.body; @@ -764,7 +1113,16 @@ function prepareComment(comment: { id: number; created_at: string; body: string; } const commentType = isReviewComment(comment, eventType) ? 'review' as const : 'issue' as const; - const unprocessedComment: UnprocessedComment = { id: comment.id, createdAt: comment.created_at, body: enhancedBody, author: commentAuthor, type: commentType, hasCodeContext: commentType === 'review' && !!comment.diff_hunk }; + const unprocessedComment: UnprocessedComment = { + id: comment.id, + createdAt: comment.created_at, + updatedAt: comment.updated_at, + revisionIdentity: revisionIdentity ?? getUnprocessedCommentRevisionIdentity({ updatedAt: comment.updated_at, body: comment.body, type: commentType }), + body: enhancedBody, + author: commentAuthor, + type: commentType, + hasCodeContext: commentType === 'review' && !!comment.diff_hunk, + }; return { enhancedBody, unprocessedComment, llmFromKeywords }; } @@ -812,37 +1170,46 @@ function buildCommandJobFields(commandMeta: CommandMeta): CommandJobFields { }; } -function buildPendingCommandFields(commandMeta: CommandMeta): Pick { +function buildPendingCommandFields(commandMeta: CommandMeta, modelSelection?: CanonicalModelSelection): Pick { return { ...buildCommandJobFields(commandMeta), llmOverride: (commandMeta.mode === 'switch' || commandMeta.mode === 'use') && commandMeta.models.length > 0 ? resolveModelAlias(commandMeta.models[0]) : undefined, + agentAlias: modelSelection?.agentAlias, + modelName: modelSelection?.model, + modelLabel: modelSelection?.githubLabel, }; } async function enqueueNewCommentJob(comment: { id: number; created_at: string; updated_at: string; body: string; path?: string; line?: number | null; diff_hunk?: string; pull_request_review_id?: number }, commentAuthor: string, eventContext: CommentContext, options: EnqueueCommentOptions): Promise { const { eventType, prNumber, owner, repo } = eventContext; - const { payload, redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS, correlationId, MODEL_LABEL_PATTERN = '^llm-(.+)$', commandMeta, prefetchedPRData, ultrafixMeta, commentRevisionIdentity } = options; + const { payload, redisClient, PR_FOLLOWUP_TRIGGER_KEYWORDS, correlationId, MODEL_LABEL_PATTERN = '^llm-(.+)$', commandMeta, prefetchedPRData, ultrafixMeta, commentRevisionIdentity, modelSelection, pendingOnly = false } = options; const correlatedLogger = logger.withCorrelation(correlationId); - const { unprocessedComment, llmFromKeywords } = prepareComment(comment, commentAuthor, eventType, PR_FOLLOWUP_TRIGGER_KEYWORDS); + const { unprocessedComment, llmFromKeywords } = prepareComment(comment, commentAuthor, eventType, { + keywords: PR_FOLLOWUP_TRIGGER_KEYWORDS, + revisionIdentity: commentRevisionIdentity, + }); const { branchName, prLabels } = prefetchedPRData || await getPRBranchAndLabels(eventType, payload, { owner, repo, prNumber }); - const llm = resolveLlm(llmFromKeywords, prLabels, { modelLabelPattern: MODEL_LABEL_PATTERN, prNumber, correlatedLogger, commandMeta }); + const llm = modelSelection?.model ?? resolveLlm(llmFromKeywords, prLabels, { modelLabelPattern: MODEL_LABEL_PATTERN, prNumber, correlatedLogger, commandMeta }); const jobData: CommentJobData = { - pullRequestNumber: prNumber, comments: [unprocessedComment], repoOwner: owner, repoName: repo, branchName, llm, correlationId: generateCorrelationId(), + pullRequestNumber: prNumber, comments: pendingOnly ? [] : [unprocessedComment], repoOwner: owner, repoName: repo, branchName, llm, correlationId: generateCorrelationId(), + ...(modelSelection ? { agentAlias: modelSelection.agentAlias, modelName: modelSelection.model, modelLabel: modelSelection.githubLabel } : {}), ...(commandMeta ? { ...buildCommandJobFields(commandMeta), commandCommentId: comment.id, commandCommentCreatedAt: comment.created_at, + commandCommentUpdatedAt: comment.updated_at, + commandCommentRevisionIdentity: unprocessedComment.revisionIdentity, commandCommentType: unprocessedComment.type, } : {}), ...(ultrafixMeta ? { ultrafixMeta } : {}), }; - const manualCommand = commandMeta?.mode === 'fix' || commandMeta?.mode === 'review'; + const deterministicCommand = commandMeta != null; const commentRevisionSlug = (commentRevisionIdentity ?? getCommentRevisionIdentity(comment, eventType)).replace(/[^a-zA-Z0-9_-]/g, '-'); - const jobId = manualCommand + const jobId = deterministicCommand ? `pr-comments-batch-${owner}-${repo}-${prNumber}-${comment.id}-${commentRevisionSlug}` : `pr-comments-batch-${owner}-${repo}-${prNumber}-${Date.now()}`; const commentTrackingKey = `pr-comment-processed:${owner}:${repo}:${prNumber}:${comment.id}`; diff --git a/packages/core/src/webhook/slashCommandParser.ts b/packages/core/src/webhook/slashCommandParser.ts index a13a23389..d8b8245eb 100644 --- a/packages/core/src/webhook/slashCommandParser.ts +++ b/packages/core/src/webhook/slashCommandParser.ts @@ -46,7 +46,7 @@ export interface SwitchCommandMeta { export interface UseCommandMeta { mode: 'use'; - /** Target model labels for single-run override */ + /** Target model label for the durable PR model transition */ models: string[]; /** Extra instructions from lines below the command */ instructions: string; diff --git a/src/jobs/prCommentAgentUtils.ts b/src/jobs/prCommentAgentUtils.ts index a917aa3de..8ded56013 100644 --- a/src/jobs/prCommentAgentUtils.ts +++ b/src/jobs/prCommentAgentUtils.ts @@ -1,5 +1,5 @@ import type { Logger } from 'pino'; -import { AgentRegistry, resolveConfiguredModel, resolveLlmLabel, runLightweightLLMAnalysis } from '@propr/core'; +import { AgentRegistry, resolveCanonicalModelSelectionFromLabels, resolveConfiguredModel, resolveLlmLabel, runLightweightLLMAnalysis } from '@propr/core'; import { loadSettings, loadSummarizationSettings, NoDefaultModelConfiguredError } from '@propr/core'; import type { AnalysisResult, ClaudeCodeResponse } from '@propr/core'; import type { WorkerStateManager } from '@propr/core'; @@ -262,8 +262,57 @@ export async function resolvePRCommentModelName(llm: string | null | undefined, return modelName; } +interface ProviderLimitRetryContext { + isRetryFromRateLimit?: boolean; + agentAlias?: string; + modelName?: string; + pullRequestNumber: number; + correlatedLogger: Logger; +} + +export async function isProviderLimitRetrySuperseded( + labels: Array, + context: ProviderLimitRetryContext, +): Promise { + if (!context.isRetryFromRateLimit || !context.agentAlias || !context.modelName) return false; + const modelLabelPattern = process.env.MODEL_LABEL_PATTERN || '^llm-(.+)$'; + const managedLabels = (await Promise.all(labels.map(async label => { + const name = typeof label === 'string' ? label : label.name; + const selection = await resolveCanonicalModelSelectionFromLabels([label], modelLabelPattern); + return new RegExp(modelLabelPattern).test(name) || selection ? name : null; + }))).filter((label): label is string => label !== null); + if (managedLabels.length > 1) { + context.correlatedLogger.info({ + pullRequestNumber: context.pullRequestNumber, + retryAgent: context.agentAlias, + retryModel: context.modelName, + managedLabels, + }, 'Skipping provider-limit retry while durable PR model labels are transitioning'); + return true; + } + const liveSelection = await resolveCanonicalModelSelectionFromLabels( + labels, + modelLabelPattern, + ); + if (!liveSelection || ( + liveSelection.agentAlias.toLowerCase() === context.agentAlias.toLowerCase() + && liveSelection.model.toLowerCase() === context.modelName.toLowerCase() + )) return false; + + context.correlatedLogger.info({ + pullRequestNumber: context.pullRequestNumber, + retryAgent: context.agentAlias, + retryModel: context.modelName, + liveAgent: liveSelection.agentAlias, + liveModel: liveSelection.model, + }, 'Skipping provider-limit retry superseded by a newer durable PR model selection'); + return true; +} + export interface AgentExecutionParams { llm: string | null | undefined; + agentAlias?: string; + modelName?: string; worktreePath: string; branchName: string; prompt: string; @@ -279,7 +328,7 @@ export interface AgentExecutionParams { } export async function resolveAndExecuteAgent(params: AgentExecutionParams): Promise<{ claudeResult: ClaudeCodeResponse; agentType: string }> { - const { llm, worktreePath, branchName, prompt, pullRequestNumber, repoOwner, repoName, taskId, stateManager, correlatedLogger, githubToken, redisClient, reasoningLevel } = params; + const { llm, agentAlias: explicitAgentAlias, modelName: explicitModelName, worktreePath, branchName, prompt, pullRequestNumber, repoOwner, repoName, taskId, stateManager, correlatedLogger, githubToken, redisClient, reasoningLevel } = params; const registry = AgentRegistry.getInstance(); await registry.ensureInitialized(); @@ -287,7 +336,10 @@ export async function resolveAndExecuteAgent(params: AgentExecutionParams): Prom let agentAlias: string; let modelToUse: string; - if (llm) { + if (explicitAgentAlias && explicitModelName) { + agentAlias = explicitAgentAlias; + modelToUse = explicitModelName; + } else if (llm) { const resolution = await resolveLlmLabel(llm); agentAlias = resolution.agentAlias; modelToUse = resolution.model; diff --git a/src/jobs/prCommentCommandContext.ts b/src/jobs/prCommentCommandContext.ts index 95497994f..d58331747 100644 --- a/src/jobs/prCommentCommandContext.ts +++ b/src/jobs/prCommentCommandContext.ts @@ -1,15 +1,26 @@ import type { Logger } from 'pino'; -import type { CommentJobData, UnprocessedComment } from '@propr/core'; +import { + dedupeUnprocessedComments, + getUnprocessedCommentRevisionIdentity, + type CommentJobData, + type UnprocessedComment, +} from '@propr/core'; interface CommentChronology { id: number; createdAt?: string; + updatedAt?: string; + revisionIdentity?: string; type?: UnprocessedComment['type']; + ingestionOrder?: number; } interface ModelOverride extends CommentChronology { commandMode?: UnprocessedComment['commandMode']; llmOverride?: string | null; + agentAlias?: string; + modelName?: string; + modelLabel?: string; } interface PendingCommandContext { @@ -27,7 +38,36 @@ function compareCommentTypes(left: CommentChronology['type'], right: CommentChro return left === 'issue' ? -1 : 1; } +function compareOptionalStrings(left: string | undefined, right: string | undefined): number { + if (left === right) return 0; + if (left === undefined) return -1; + if (right === undefined) return 1; + return left.localeCompare(right); +} + +function compareOptionalNumbers(left: number | undefined, right: number | undefined): number { + if (left === right) return 0; + if (left === undefined) return -1; + if (right === undefined) return 1; + return left - right; +} + +function compareCommentRevisions(left: CommentChronology, right: CommentChronology): number { + const updateOrder = compareOptionalStrings( + left.updatedAt ?? left.createdAt, + right.updatedAt ?? right.createdAt, + ); + if (updateOrder !== 0) return updateOrder; + + const ingestionOrder = compareOptionalNumbers(left.ingestionOrder, right.ingestionOrder); + return ingestionOrder !== 0 + ? ingestionOrder + : compareOptionalStrings(left.revisionIdentity, right.revisionIdentity); +} + function compareCommentChronology(left: CommentChronology, right: CommentChronology, useCreatedAt: boolean): number { + if (left.id === right.id && left.type === right.type) return compareCommentRevisions(left, right); + if (useCreatedAt && left.createdAt !== undefined && right.createdAt !== undefined) { const createdAtOrder = left.createdAt.localeCompare(right.createdAt); if (createdAtOrder !== 0) return createdAtOrder; @@ -42,6 +82,29 @@ function compareCommentChronology(left: CommentChronology, right: CommentChronol : compareCommentTypes(left.type, right.type); } +function orderedComments(comments: UnprocessedComment[]): Array { + return comments.map((comment, ingestionOrder) => ({ + ...comment, + revisionIdentity: getUnprocessedCommentRevisionIdentity(comment), + ingestionOrder, + })); +} + +function latestCommentRevisions(comments: Array): Array { + const latestByComment = new Map(); + for (const comment of comments) { + const key = `${comment.type}:${comment.id}`; + const latest = latestByComment.get(key); + if (!latest || compareCommentChronology(comment, latest, true) > 0) latestByComment.set(key, comment); + } + return [...latestByComment.values()]; +} + +function latestCommentsForExecution(comments: UnprocessedComment[]): UnprocessedComment[] { + return latestCommentRevisions(orderedComments(comments)) + .map(comment => comments[comment.ingestionOrder!]); +} + function findLatestComment( comments: T[], predicate: (comment: T) => boolean, @@ -57,13 +120,23 @@ function getQueuedCommandChronologyCandidates(jobData: CommentJobData): CommentC if (!jobData.commandMode || jobData.commandMode === 'default') return []; if (jobData.commandCommentId !== undefined) { - const ownerComment = jobData.comments?.find(comment => + const candidateComments = orderedComments(jobData.comments ?? []).filter(comment => comment.id === jobData.commandCommentId && (jobData.commandCommentType === undefined || comment.type === jobData.commandCommentType)); + const ownerComment = candidateComments.find(comment => + jobData.commandCommentRevisionIdentity !== undefined + && comment.revisionIdentity === jobData.commandCommentRevisionIdentity) + ?? candidateComments.find(comment => + jobData.commandCommentUpdatedAt !== undefined + && comment.updatedAt === jobData.commandCommentUpdatedAt) + ?? candidateComments[0]; return [{ id: jobData.commandCommentId, createdAt: jobData.commandCommentCreatedAt ?? ownerComment?.createdAt, + updatedAt: jobData.commandCommentUpdatedAt ?? ownerComment?.updatedAt, + revisionIdentity: jobData.commandCommentRevisionIdentity ?? ownerComment?.revisionIdentity, type: jobData.commandCommentType ?? ownerComment?.type, + ingestionOrder: ownerComment?.ingestionOrder, }]; } @@ -75,7 +148,10 @@ function getQueuedCommandChronologyCandidates(jobData: CommentJobData): CommentC function resolvePendingCommandContext(jobData: CommentJobData, commentsToProcess: UnprocessedComment[]): PendingCommandContext { const queuedCommandCandidates = getQueuedCommandChronologyCandidates(jobData); - const pendingChronologyRecords = commentsToProcess.filter(comment => + // Multiple webhook deliveries may contain revisions of one GitHub comment. + // Only its newest revision participates in routing; all revisions remain in + // jobData.comments for durable retry/recovery. + const pendingChronologyRecords = latestCommentRevisions(orderedComments(commentsToProcess)).filter(comment => (!!comment.commandMode && comment.commandMode !== 'default') || comment.llmOverride !== undefined); const useCreatedAt = [...queuedCommandCandidates, ...pendingChronologyRecords] @@ -91,14 +167,14 @@ function resolvePendingCommandContext(jobData: CommentJobData, commentsToProcess queuedCommandChronology === undefined || compareChronology(comment, queuedCommandChronology) > 0; const latestCommandComment = findLatestComment( - commentsToProcess, + pendingChronologyRecords, comment => !!comment.commandMode && comment.commandMode !== 'default' && isNewerThanQueuedCommand(comment), compareChronology, ); const latestPendingOverrideComment = findLatestComment( - commentsToProcess, + pendingChronologyRecords, comment => comment.llmOverride !== undefined && isNewerThanQueuedCommand(comment), compareChronology, @@ -115,6 +191,9 @@ function getQueuedOverride(jobData: CommentJobData, queuedCommandChronology: Com ...queuedCommandChronology, commandMode: 'use', llmOverride: jobData.requestedModels?.[0] ?? jobData.llm, + agentAlias: jobData.agentAlias, + modelName: jobData.modelName, + modelLabel: jobData.modelLabel, } : undefined; } @@ -125,12 +204,20 @@ function applyCommandComment(jobData: CommentJobData, comment: UnprocessedCommen jobData.commandInstructions = comment.commandInstructions; jobData.commandCommentId = comment.id; jobData.commandCommentCreatedAt = comment.createdAt; + jobData.commandCommentUpdatedAt = comment.updatedAt; + jobData.commandCommentRevisionIdentity = getUnprocessedCommentRevisionIdentity(comment); jobData.commandCommentType = comment.type; jobData.ultrafixMeta = comment.ultrafixMeta; + jobData.agentAlias = comment.agentAlias; + jobData.modelName = comment.modelName; + jobData.modelLabel = comment.modelLabel; } function applyModelOverride(jobData: CommentJobData, latestCommandComment: UnprocessedComment | undefined, latestOverrideComment: ModelOverride | undefined): void { if (latestOverrideComment?.llmOverride !== undefined) jobData.llm = latestOverrideComment.llmOverride; + if (latestOverrideComment?.agentAlias) jobData.agentAlias = latestOverrideComment.agentAlias; + if (latestOverrideComment?.modelName) jobData.modelName = latestOverrideComment.modelName; + if (latestOverrideComment?.modelLabel) jobData.modelLabel = latestOverrideComment.modelLabel; if ( latestCommandComment?.commandMode === 'review' && !latestCommandComment.requestedModels?.length @@ -141,14 +228,18 @@ function applyModelOverride(jobData: CommentJobData, latestCommandComment: Unpro } } -export function applyPendingCommentCommandContext(jobData: CommentJobData, commentsToProcess: UnprocessedComment[], correlatedLogger: Logger): void { +export function applyPendingCommentCommandContext(jobData: CommentJobData, commentsToProcess: UnprocessedComment[], correlatedLogger: Logger): UnprocessedComment[] { + // The worker owns every comment it claimed. Persist that complete ordered + // set before routing checks, provider retries, or superseded exits. + jobData.comments = dedupeUnprocessedComments(commentsToProcess); + const commentsForExecution = latestCommentsForExecution(jobData.comments); const { queuedCommandChronology, latestCommandComment, latestPendingOverrideComment, - } = resolvePendingCommandContext(jobData, commentsToProcess); + } = resolvePendingCommandContext(jobData, jobData.comments); - if (!latestCommandComment && !latestPendingOverrideComment) return; + if (!latestCommandComment && !latestPendingOverrideComment) return commentsForExecution; const latestOverrideComment: ModelOverride | undefined = latestPendingOverrideComment ?? getQueuedOverride(jobData, queuedCommandChronology); @@ -161,8 +252,10 @@ export function applyPendingCommentCommandContext(jobData: CommentJobData, comme llmOverride: latestOverrideComment?.llmOverride, commandCommentId: latestCommandComment?.id, commandCommentCreatedAt: latestCommandComment?.createdAt, + commandCommentUpdatedAt: latestCommandComment?.updatedAt, queuedCommandCommentId: queuedCommandChronology?.id, queuedCommandCommentCreatedAt: queuedCommandChronology?.createdAt, overrideCommentId: latestOverrideComment?.id, }, 'Applied command context from pending batched comment'); + return commentsForExecution; } diff --git a/src/jobs/prCommentJobTypes.ts b/src/jobs/prCommentJobTypes.ts new file mode 100644 index 000000000..32d953959 --- /dev/null +++ b/src/jobs/prCommentJobTypes.ts @@ -0,0 +1,86 @@ +import type { Job } from 'bullmq'; +import type { Logger } from 'pino'; +import type { + ClaudeCodeResponse, + CommentJobData, + UnprocessedComment, + WorkerStateManager, + WorktreeInfo, + getAuthenticatedOctokit, +} from '@propr/core'; + +export interface PRData { + data: { + head: { ref: string; sha?: string }; + body: string | null; + labels: Array<{ name: string }>; + user: { login: string }; + title: string; + }; +} + +export interface PRComment { + id: number; + body: string; + body_html?: string; + user: { login: string; type?: string }; + created_at: string; + pull_request_review_id?: number; +} + +export interface PRJobContext { + pullRequestNumber: number; + jobBranchName: string | undefined; + repoOwner: string; + repoName: string; + llm: string | null | undefined; + agentAlias?: string; + modelName?: string; + modelLabel?: string; + isRetryFromRateLimit?: boolean; + correlationId: string; + correlatedLogger: Logger; + primaryProcessingLabels: string[]; + isBatchJob: boolean; + commentsToProcess: UnprocessedComment[]; + pickedUpComments: UnprocessedComment[]; + originalUltrafixMeta: CommentJobData['ultrafixMeta']; +} + +export interface ValidationResult { + skip: boolean; + reason?: string; + prData?: PRData; + validatedComments?: UnprocessedComment[]; + unprocessedComments?: UnprocessedComment[]; + llm?: string | null; + prCommentsForValidation?: PRComment[]; +} + +export interface LockParams { + lockKey: string; + lockToken: string; + correlatedLogger: Logger; + job: Job; +} + +export interface ProcessingState { + octokit: Awaited> | null; + localRepoPath: string | undefined; + worktreeInfo: WorktreeInfo | undefined; + claudeResult: ClaudeCodeResponse | null; + authorsText: string; + unprocessedComments: UnprocessedComment[]; + startingWorkComment: { data: { id: number; html_url: string } } | null; +} + +export interface ExecuteProcessingParams { + job: Job; + context: PRJobContext; + llm: string | null | undefined; + taskId: string; + stateManager: WorkerStateManager; + state: ProcessingState; + lockKey: string; + lockToken: string; +} diff --git a/src/jobs/prCommentJobUtils.ts b/src/jobs/prCommentJobUtils.ts index ccd76e874..6325d3a95 100644 --- a/src/jobs/prCommentJobUtils.ts +++ b/src/jobs/prCommentJobUtils.ts @@ -5,6 +5,8 @@ import { generateCorrelationId, handleError, getAuthenticatedOctokit, cleanupWorktree, formatResetTime, recordLLMMetrics, issueQueue, TaskStates, getDefaultModel, resolveModelAlias, getPendingPrCommentsKey, + resolveCanonicalModelSelectionFromLabels, + withUltrafixLabelTransition, describeAgentTermination, resolveAgentTerminationReason, type WorktreeInfo, type ClaudeCodeResponse, type ClaudeResult, type CommentJobData, type UnprocessedComment, type WorkerStateManager, @@ -15,6 +17,8 @@ import { extractModelLabelToken } from './prModelLabelUtils.js'; import { buildWorkEvidenceMarker, filterRealComments } from '../shared/workEvidenceMarker.js'; import type { ReasoningLevel } from '@propr/shared'; import { releasePRProcessingLock } from './prProcessingLock.js'; +import { buildProviderLimitRetryJobData } from './prCommentRouting.js'; +export { buildProviderLimitRetryJobData } from './prCommentRouting.js'; export function toClaudeResult(response: ClaudeCodeResponse): ClaudeResult { return { @@ -167,6 +171,38 @@ export interface JobErrorOptions { startingWorkComment: { data: { id: number } } | null; claudeResult: ClaudeCodeResponse | null; correlationId: string; correlatedLogger: Logger; stateManager: WorkerStateManager; taskId: string; + redisClient: Redis; + /** Actual in-memory routing used by this attempt; never persisted for ordinary jobs. */ + runtimeAgentAlias?: string; runtimeModelName?: string; +} + +interface ProviderLimitRetryRouting { + runtimeAgentAlias?: string; + runtimeModelName?: string; +} + +/** Persist the concrete provider route used by the failed attempt. */ +export function buildRuntimeProviderLimitRetryJobData( + jobData: CommentJobData, + routing: ProviderLimitRetryRouting, +): CommentJobData { + const retryData = buildProviderLimitRetryJobData(jobData); + if (!routing.runtimeAgentAlias || !routing.runtimeModelName) return retryData; + + return { + ...retryData, + agentAlias: routing.runtimeAgentAlias, + modelName: routing.runtimeModelName, + llm: routing.runtimeModelName, + }; +} + +/** Build a deterministic retry identity from the persisted concrete route. */ +export function buildProviderLimitRetryJobId(jobData: CommentJobData): string { + const routingSlug = `${jobData.agentAlias || 'default'}-${jobData.modelName || jobData.llm || 'default'}` + .replace(/[^a-zA-Z0-9-]/g, '-'); + const branchSlug = (jobData.branchName || 'main').replace(/[^a-zA-Z0-9-]/g, '-').slice(0, 30); + return `pr-comments-batch-${jobData.repoOwner}-${jobData.repoName}-${jobData.pullRequestNumber}-${routingSlug}-${branchSlug}-ratelimit-retry`; } export class UsageLimitError extends Error { @@ -198,31 +234,77 @@ async function postCancellationComment(params: CancellationCommentParams): Promi } } -async function handleUsageLimitError(error: UsageLimitError, job: Job, options: JobErrorOptions): Promise { - const { pullRequestNumber, repoOwner, repoName, authorsText, octokit, correlatedLogger } = options; +async function handleUsageLimitError(error: UsageLimitError, job: Job, options: JobErrorOptions): Promise<'requeued' | 'provider_limit_retry_superseded'> { + const { pullRequestNumber, repoOwner, repoName, authorsText, octokit, correlatedLogger, redisClient } = options; correlatedLogger.warn({ pullRequestNumber, resetTimestamp: error.resetTimestamp }, 'Claude usage limit hit during PR comment processing. Requeueing job.'); const resetTimeUTC = error.resetTimestamp ? (error.resetTimestamp * 1000) : (Date.now() + 60 * 60 * 1000); const delay = (resetTimeUTC - Date.now()) + REQUEUE_BUFFER_MS + Math.floor(Math.random() * REQUEUE_JITTER_MS); const readableResetTime = formatResetTime(error.resetTimestamp); - // Use deterministic jobId to prevent duplicate jobs if requeue is triggered multiple times - const llmSlug = (job.data.llm || 'default').replace(/[^a-zA-Z0-9-]/g, '-'); - const branchSlug = (job.data.branchName || 'main').replace(/[^a-zA-Z0-9-]/g, '-').slice(0, 30); - const requeueJobId = `pr-comments-batch-${repoOwner}-${repoName}-${pullRequestNumber}-${llmSlug}-${branchSlug}-ratelimit-retry`; - - if (octokit) { - try { - await octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { - owner: repoOwner, repo: repoName, issue_number: pullRequestNumber, - body: `⌛ **Processing Delayed:** Claude's usage limit was reached while processing requests from ${authorsText}.\n\nThe job has been automatically rescheduled and will restart ${readableResetTime}.\n\n---\n*Job ID: ${requeueJobId} will run again after delay.*` - }); - } catch (commentError) { - correlatedLogger.error({ error: (commentError as Error).message }, 'Failed to post usage limit delay comment to PR.'); - } - } - - await issueQueue.add(job.name, job.data, { jobId: requeueJobId, delay: Math.max(0, delay) }); + return withUltrafixLabelTransition( + redisClient, + { owner: repoOwner, repo: repoName, pr: pullRequestNumber }, + async lease => { + // `/use` holds this same per-PR lease through label convergence and + // its queue snapshot. Re-read the canonical selection while holding + // it, then publish before releasing it, so a retry is either visible + // to `/use` or observes the newly selected provider and yields. + const retryJobData = buildRuntimeProviderLimitRetryJobData(job.data, options); + const retryAgentAlias = retryJobData.agentAlias; + const retryModelName = retryJobData.modelName; + if (octokit && retryAgentAlias && retryModelName) { + try { + const response = await octokit.request('GET /repos/{owner}/{repo}/pulls/{pull_number}', { + owner: repoOwner, + repo: repoName, + pull_number: pullRequestNumber, + }) as { data: { labels: Array<{ name: string }> } }; + const liveSelection = await resolveCanonicalModelSelectionFromLabels( + response.data.labels, + process.env.MODEL_LABEL_PATTERN || '^llm-(.+)$', + ); + if (liveSelection) { + if ( + liveSelection.agentAlias.toLowerCase() !== retryAgentAlias.toLowerCase() + || liveSelection.model.toLowerCase() !== retryModelName.toLowerCase() + ) { + correlatedLogger.info({ + pullRequestNumber, + retryAgent: retryAgentAlias, + retryModel: retryModelName, + liveAgent: liveSelection.agentAlias, + liveModel: liveSelection.model, + }, 'Provider-limit retry superseded by a newer durable PR model selection'); + return 'provider_limit_retry_superseded'; + } + retryJobData.modelLabel = liveSelection.githubLabel; + } + } catch (selectionError) { + correlatedLogger.warn({ error: (selectionError as Error).message }, 'Could not verify live PR model before provider-limit requeue; preserving retry'); + } + } + + // Use deterministic jobId to prevent duplicate jobs if requeue is triggered multiple times + const requeueJobId = buildProviderLimitRetryJobId(retryJobData); + + if (octokit) { + try { + await octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { + owner: repoOwner, repo: repoName, issue_number: pullRequestNumber, + body: `⌛ **Processing Delayed:** Claude's usage limit was reached while processing requests from ${authorsText}.\n\nThe job has been automatically rescheduled and will restart ${readableResetTime}.\n\n---\n*Job ID: ${requeueJobId} will run again after delay.*` + }); + } catch (commentError) { + correlatedLogger.error({ error: (commentError as Error).message }, 'Failed to post usage limit delay comment to PR.'); + } + } + + await lease.assertOwned(); + await issueQueue.add(job.name, retryJobData, { jobId: requeueJobId, delay: Math.max(0, delay) }); + await lease.assertOwned(); + return 'requeued'; + }, + ); } async function handleUserCancellation(options: JobErrorOptions, errorMessage: string): Promise { @@ -260,7 +342,7 @@ async function handleGenericError(error: Error, options: JobErrorOptions): Promi } } -export async function handleJobError(error: Error, job: Job, options: JobErrorOptions): Promise { +export async function handleJobError(error: Error, job: Job, options: JobErrorOptions): Promise<'provider_limit_retry_superseded' | undefined> { const { repoOwner, repoName, octokit, startingWorkComment, correlatedLogger, stateManager, taskId } = options; const isUserCancelled = error.message?.includes('aborted by user'); @@ -275,16 +357,18 @@ export async function handleJobError(error: Error, job: Job, opt await postCancellationComment({ octokit, repoOwner, repoName, commentId: startingWorkComment.data.id, correlatedLogger }); correlatedLogger.info({ taskId, commentId: startingWorkComment.data.id }, 'Updated GitHub comment for cancelled task'); } - return; + return undefined; } if (isUsageLimit) { - await handleUsageLimitError(error as UsageLimitError, job, options); + const disposition = await handleUsageLimitError(error as UsageLimitError, job, options); + return disposition === 'provider_limit_retry_superseded' ? disposition : undefined; } else if (isUserCancelled) { await handleUserCancellation(options, error.message); } else { await handleGenericError(error, options); } + return undefined; } export interface CleanupOptions { @@ -370,11 +454,7 @@ export { buildCompletionComment } from './prCompletionComment.js'; export type { CommentContext, UndoLinkContext } from './prCompletionComment.js'; export type { PRFile } from './prFileUtils.js'; export { - fetchPRFiles, - fetchPRFileContents, - formatPRDiff, - formatPRDiffWithMetadata, - formatFileContents, - agentResultToClaudeResponse, + fetchPRFiles, fetchPRFileContents, formatPRDiff, formatPRDiffWithMetadata, + formatFileContents, agentResultToClaudeResponse, } from './prFileUtils.js'; export { applyPendingCommentCommandContext } from './prCommentCommandContext.js'; diff --git a/src/jobs/prCommentReviewJob.ts b/src/jobs/prCommentReviewJob.ts index 3a092c3a9..bad9424a6 100644 --- a/src/jobs/prCommentReviewJob.ts +++ b/src/jobs/prCommentReviewJob.ts @@ -27,21 +27,10 @@ import { } from './prTaskTitleHelpers.js'; import type { Redis } from 'ioredis'; import { buildWorkEvidenceMarker, filterRealComments } from '../shared/workEvidenceMarker.js'; +import type { PRJobContext } from './prCommentJobTypes.js'; export type { ReviewAssignment, ReviewResult } from './prReviewRunner.js'; - -export interface PRJobContext { - pullRequestNumber: number; - jobBranchName: string | undefined; - repoOwner: string; - repoName: string; - llm: string | null | undefined; - correlationId: string; - correlatedLogger: Logger; - primaryProcessingLabels: string[]; - isBatchJob: boolean; - commentsToProcess: UnprocessedComment[]; -} +export type { PRJobContext } from './prCommentJobTypes.js'; interface ProcessingState { octokit: Awaited> | null; diff --git a/src/jobs/prCommentRouting.ts b/src/jobs/prCommentRouting.ts new file mode 100644 index 000000000..d013dbe85 --- /dev/null +++ b/src/jobs/prCommentRouting.ts @@ -0,0 +1,10 @@ +import { dedupeUnprocessedComments, type CommentJobData } from '@propr/core'; + +/** Reconstruct a delayed provider retry without dropping explicit routing. */ +export function buildProviderLimitRetryJobData(jobData: CommentJobData): CommentJobData { + return { + ...jobData, + comments: dedupeUnprocessedComments(jobData.comments ?? []), + isRetryFromRateLimit: true, + }; +} diff --git a/src/jobs/prPendingComments.ts b/src/jobs/prPendingComments.ts index 3315301b4..4e0a46942 100644 --- a/src/jobs/prPendingComments.ts +++ b/src/jobs/prPendingComments.ts @@ -1,6 +1,11 @@ import type { Logger } from 'pino'; import type { UnprocessedComment } from '@propr/core'; -import { getPendingPrCommentsKey } from '@propr/core'; +import { + dedupeUnprocessedComments, + getPendingPrCommentsKey, + getUnprocessedCommentIdentity, + restorePendingCommentsIdempotently, +} from '@propr/core'; import type { Redis } from 'ioredis'; export { applyPendingCommentCommandContext } from './prCommentCommandContext.js'; @@ -15,10 +20,12 @@ export function parsePendingComment(commentJson: string, correlatedLogger: Logge } export function processPendingComments(commentsToProcess: UnprocessedComment[], pendingComments: string[], correlatedLogger: Logger): void { + const seen = new Set(commentsToProcess.map(getUnprocessedCommentIdentity)); for (const commentJson of pendingComments) { const pendingComment = parsePendingComment(commentJson, correlatedLogger); - if (pendingComment && !commentsToProcess.some(c => c.id === pendingComment.id)) { + if (pendingComment && !seen.has(getUnprocessedCommentIdentity(pendingComment))) { commentsToProcess.push(pendingComment); + seen.add(getUnprocessedCommentIdentity(pendingComment)); } } } @@ -28,14 +35,45 @@ export interface PendingCommentPickup { pickedUpComments: UnprocessedComment[]; } -export async function pickUpPendingCommentsWithClaim(commentsToProcess: UnprocessedComment[], options: { repoOwner: string; repoName: string; pullRequestNumber: number; correlatedLogger: Logger; redisClient: Redis }): Promise { - const { repoOwner, repoName, pullRequestNumber, correlatedLogger, redisClient } = options; +const CLAIM_PENDING_COMMENTS_SCRIPT = ` +if redis.call('EXISTS', KEYS[2]) == 1 then + return redis.call('LRANGE', KEYS[2], 0, -1) +end +local pending = redis.call('LRANGE', KEYS[1], 0, -1) +if #pending > 0 then + redis.call('DEL', KEYS[1]) + for _, value in ipairs(pending) do redis.call('RPUSH', KEYS[2], value) end + redis.call('EXPIRE', KEYS[2], 86400) +end +return pending +`; + +function pendingCommentClaimKey(pendingCommentsKey: string, claimId: string): string { + return `${pendingCommentsKey}:claim:${claimId}`; +} + +export async function acknowledgePendingCommentClaim( + options: { repoOwner: string; repoName: string; pullRequestNumber: number; claimId: string; redisClient: Redis }, +): Promise { + const pendingCommentsKey = getPendingPrCommentsKey(options.repoOwner, options.repoName, options.pullRequestNumber); + await options.redisClient.del(pendingCommentClaimKey(pendingCommentsKey, options.claimId)); +} + +export async function pickUpPendingCommentsWithClaim(commentsToProcess: UnprocessedComment[], options: { repoOwner: string; repoName: string; pullRequestNumber: number; correlatedLogger: Logger; redisClient: Redis; claimId?: string }): Promise { + const { repoOwner, repoName, pullRequestNumber, correlatedLogger, redisClient, claimId } = options; const pendingCommentsKey = getPendingPrCommentsKey(repoOwner, repoName, pullRequestNumber); - const originalCommentIds = new Set(commentsToProcess.map(comment => comment.id)); + const originalCommentIds = new Set(commentsToProcess.map(getUnprocessedCommentIdentity)); try { - const pendingComments = await redisClient.lrange(pendingCommentsKey, 0, -1); + const pendingComments = claimId + ? await redisClient.eval( + CLAIM_PENDING_COMMENTS_SCRIPT, + 2, + pendingCommentsKey, + pendingCommentClaimKey(pendingCommentsKey, claimId), + ) as string[] + : await redisClient.lrange(pendingCommentsKey, 0, -1); if (pendingComments.length > 0) { - await redisClient.del(pendingCommentsKey); + if (!claimId) await redisClient.del(pendingCommentsKey); processPendingComments(commentsToProcess, pendingComments, correlatedLogger); correlatedLogger.info({ pullRequestNumber, pendingCount: pendingComments.length, totalCount: commentsToProcess.length }, 'Picked up pending comments from Redis'); } @@ -43,8 +81,8 @@ export async function pickUpPendingCommentsWithClaim(commentsToProcess: Unproces correlatedLogger.warn({ error: (redisError as Error).message }, 'Failed to fetch pending comments from Redis'); } return { - commentsToProcess, - pickedUpComments: commentsToProcess.filter(comment => !originalCommentIds.has(comment.id)), + commentsToProcess: dedupeUnprocessedComments(commentsToProcess), + pickedUpComments: commentsToProcess.filter(comment => !originalCommentIds.has(getUnprocessedCommentIdentity(comment))), }; } @@ -54,10 +92,19 @@ export async function pickUpPendingComments(commentsToProcess: UnprocessedCommen /** Return comments claimed by a cancelled job to the head of the shared pending list. */ export async function restorePendingComments(comments: UnprocessedComment[], options: { repoOwner: string; repoName: string; pullRequestNumber: number; redisClient: Redis }): Promise { - if (comments.length === 0) return; const { repoOwner, repoName, pullRequestNumber, redisClient } = options; const pendingCommentsKey = getPendingPrCommentsKey(repoOwner, repoName, pullRequestNumber); - const serializedComments = comments.map(comment => JSON.stringify(comment)).reverse(); - await redisClient.lpush(pendingCommentsKey, ...serializedComments); - await redisClient.expire(pendingCommentsKey, 3600); + await restorePendingCommentsIdempotently(redisClient, pendingCommentsKey, comments); +} + +/** Restore a stale provider retry and clear its routing before cleanup queues a successor. */ +export async function restoreSupersededProviderLimitComments( + context: { commentsToProcess: UnprocessedComment[]; llm: string | null | undefined; agentAlias?: string; modelName?: string; modelLabel?: string }, + options: { repoOwner: string; repoName: string; pullRequestNumber: number; redisClient: Redis }, +): Promise { + await restorePendingComments(context.commentsToProcess, options); + context.llm = null; + context.agentAlias = undefined; + context.modelName = undefined; + context.modelLabel = undefined; } diff --git a/src/jobs/processPullRequestCommentJob.ts b/src/jobs/processPullRequestCommentJob.ts index 9b8bdb26d..8d8387091 100644 --- a/src/jobs/processPullRequestCommentJob.ts +++ b/src/jobs/processPullRequestCommentJob.ts @@ -1,14 +1,11 @@ import { Job } from 'bullmq'; import type { Logger } from 'pino'; -import { findRunningDockerContainerForTask, getAuthenticatedOctokit, hashTaskAttemptToken, inspectLegacyDockerContainerLivenessForTask, logger, retryConfigs, runWithExecutionAbortSignal, withRetry } from '@propr/core'; +import { AgentRegistry, findRunningDockerContainerForTask, getAuthenticatedOctokit, hashTaskAttemptToken, inspectLegacyDockerContainerLivenessForTask, logger, resolveLlmLabel, retryConfigs, runWithExecutionAbortSignal, withRetry } from '@propr/core'; import { getStateManager, TaskStates } from '@propr/core'; -import type { WorkerStateManager } from '@propr/core'; import { ensureRepoCloned, createWorktreeFromExistingBranch, getRepoUrl } from '@propr/core'; -import type { WorktreeInfo } from '@propr/core'; import { ensureGitRepository } from '@propr/core'; import { createLogFiles } from '@propr/core'; import { UsageLimitError } from '@propr/core'; -import type { ClaudeCodeResponse } from '@propr/core'; import { recordLLMMetrics } from '@propr/core'; import { issueQueue, type CommentJobData, type UnprocessedComment, type JobResult } from '@propr/core'; import { Redis } from 'ioredis'; @@ -22,9 +19,14 @@ import { buildCombinedComment, extractModelFromLabels, fetchAllComments, buildPrompt, handleJobError, cleanupJob, toClaudeResult } from './prCommentJobUtils.js'; -import { pickUpPendingCommentsWithClaim, applyPendingCommentCommandContext } from './prPendingComments.js'; +import { + acknowledgePendingCommentClaim, + pickUpPendingCommentsWithClaim, + applyPendingCommentCommandContext, + restoreSupersededProviderLimitComments, +} from './prPendingComments.js'; import { executeReviewProcessing } from './prCommentReviewJob.js'; -import { generateSummaryTitle, resolveAndExecuteAgent, resolvePRCommentModelName } from './prCommentAgentUtils.js'; +import { generateSummaryTitle, isProviderLimitRetrySuperseded, resolveAndExecuteAgent, resolveDefaultAgentAndModel, resolvePRCommentModelName } from './prCommentAgentUtils.js'; import { isReviewComment } from './reviewCommentFormatter.js'; import { hasAuthorizedFixFeedback, prepareFixReviewFeedback } from './reviewFindingSelector.js'; import { retainOriginalScope } from './ultrafixOrchestrationService.js'; @@ -52,6 +54,14 @@ import { releasePRProcessingLock, startPRProcessingLockHeartbeat, } from './prProcessingLock.js'; +import type { + ExecuteProcessingParams, + LockParams, + PRData, + PRJobContext, + ProcessingState, + ValidationResult, +} from './prCommentJobTypes.js'; const redisClient = new Redis({ host: process.env.REDIS_HOST || '127.0.0.1', @@ -59,49 +69,6 @@ const redisClient = new Redis({ maxRetriesPerRequest: null, enableReadyCheck: false, }); -interface PRData { data: { head: { ref: string; sha?: string }; body: string | null; labels: Array<{ name: string }>; user: { login: string }; title: string } } -interface PRComment { id: number; body: string; body_html?: string; user: { login: string; type?: string }; created_at: string; pull_request_review_id?: number } - -interface PRJobContext { - pullRequestNumber: number; - jobBranchName: string | undefined; - repoOwner: string; - repoName: string; - llm: string | null | undefined; - correlationId: string; - correlatedLogger: Logger; - primaryProcessingLabels: string[]; - isBatchJob: boolean; - commentsToProcess: UnprocessedComment[]; -} - -interface ValidationResult { - skip: boolean; - reason?: string; - prData?: PRData; - validatedComments?: UnprocessedComment[]; - unprocessedComments?: UnprocessedComment[]; - llm?: string | null; - prCommentsForValidation?: PRComment[]; -} - -interface LockParams { - lockKey: string; - lockToken: string; - correlatedLogger: Logger; - job: Job; -} - -interface ProcessingState { - octokit: Awaited> | null; - localRepoPath: string | undefined; - worktreeInfo: WorktreeInfo | undefined; - claudeResult: ClaudeCodeResponse | null; - authorsText: string; - unprocessedComments: UnprocessedComment[]; - startingWorkComment: { data: { id: number; html_url: string } } | null; -} - async function getPrimaryLabels(): Promise { try { if (process.env.CONFIG_REPO) return await loadPrimaryProcessingLabels(); @@ -110,14 +77,12 @@ async function getPrimaryLabels(): Promise { } // Fallback to environment variable or default const envLabels = process.env.PRIMARY_PROCESSING_LABELS; - if (envLabels) { - return envLabels.split(',').map(l => l.trim()).filter(l => l); - } + if (envLabels) return envLabels.split(',').map(l => l.trim()).filter(l => l); // Final fallback to PR_LABEL for backwards compatibility return [process.env.PR_LABEL || 'propr']; } -async function initializePRJobContext(job: Job): Promise { +async function initializePRJobContext(job: Job): Promise { const { pullRequestNumber, commentId, commentBody, commentAuthor, comments, repoOwner, repoName, correlationId, ultrafixMeta: originalUltrafixMeta } = job.data; const correlatedLogger = logger.withCorrelation(correlationId); @@ -131,10 +96,13 @@ async function initializePRJobContext(job: Job): Promise { @@ -156,6 +124,9 @@ async function validatePRAndComments(octokit: Awaited primaryProcessingLabels.includes(label.name))) return { skip: true, reason: 'missing_required_label' }; - const llm = extractModelFromLabels(prData.data.labels, initialLlm, pullRequestNumber, correlatedLogger); + // A slash-command selection is persisted independently of the label so a + // reconstructed retry cannot drift providers. Ordinary jobs continue to + // derive routing from the durable PR label. + const llm = context.agentAlias && context.modelName + ? context.modelName + : extractModelFromLabels(prData.data.labels, initialLlm, pullRequestNumber, correlatedLogger); const unprocessedComments = filterUnprocessedComments(validatedComments, prCommentsForValidation, botUsername, { pullRequestNumber, correlatedLogger }); if (unprocessedComments.length === 0) return { skip: true, reason: 'already_processed' }; return { skip: false, prData, validatedComments, unprocessedComments, llm, prCommentsForValidation }; } -interface ExecuteProcessingParams { - job: Job; - context: PRJobContext; - llm: string | null | undefined; - taskId: string; - stateManager: WorkerStateManager; - state: ProcessingState; - lockKey: string; - lockToken: string; -} - function checkTerminalStateAfterExecution(currentState: { state: string } | null, taskId: string, correlatedLogger: Logger): void { const TERMINAL_STATES: string[] = [TaskStates.COMPLETED, TaskStates.FAILED, TaskStates.CANCELLED]; if (currentState && TERMINAL_STATES.includes(currentState.state)) { correlatedLogger.info({ taskId, currentState: currentState.state }, 'Task already in terminal state after agent execution, skipping state update'); - if (currentState.state === TaskStates.CANCELLED) { - throw new Error('Execution aborted by user request'); - } + if (currentState.state === TaskStates.CANCELLED) throw new Error('Execution aborted by user request'); throw new Error(`Task already in terminal state: ${currentState.state}`); } } @@ -207,7 +170,27 @@ function buildStartingWorkCommentBody(authorsText: string, unprocessedComments: return `🔄 **Starting work on follow-up changes** requested by ${authorsText}\n\nI'll analyze the ${unprocessedComments.length} request${plural} and implement the necessary changes.\n\n[View Task Progress](${taskUrl})${commentIdsSuffix}${evidenceMarker ? `\n${evidenceMarker}` : ''}`; } -async function executeProcessing(params: ExecuteProcessingParams): Promise { +interface ActiveAttemptRouting { + agentAlias?: string; + modelName?: string; +} + +async function resolveActiveAttemptRouting(context: PRJobContext, llm: string | null | undefined): Promise> { + if (context.agentAlias && context.modelName) { + return { agentAlias: context.agentAlias, modelName: context.modelName }; + } + if (llm) { + const selection = await resolveLlmLabel(llm); + return { agentAlias: selection.agentAlias, modelName: selection.model }; + } + + const registry = AgentRegistry.getInstance(); + await registry.ensureInitialized(); + const selection = await resolveDefaultAgentAndModel(registry, context.correlatedLogger); + return { agentAlias: selection.resolvedAlias, modelName: selection.resolvedModel }; +} + +async function executeProcessing(params: ExecuteProcessingParams, activeAttemptRouting: ActiveAttemptRouting): Promise { const { job, context, taskId, stateManager, state, lockKey, lockToken } = params; let { llm } = params; const { pullRequestNumber, jobBranchName, repoOwner, repoName, correlationId, correlatedLogger } = context; @@ -357,8 +340,12 @@ async function executeProcessing(params: ExecuteProcessingParams): Promise): Pr correlatedLogger.info({ pullRequestNumber, branchName: jobBranchName, llm, isBatchJob, commentsCount: commentsToProcess.length }, `Processing PR comment${isBatchJob ? 's batch' : ''} job...`); if (await restorePendingCommentsIfUltrafixJobSuperseded(job, { repoOwner, repoName, pullRequestNumber, redisClient }, context.pickedUpComments, context.originalUltrafixMeta)) return { status: 'cancelled', reason: 'ultrafix_superseded' }; - const modelName = await resolvePRCommentModelName(llm, correlatedLogger); + const modelName = context.modelName ?? await resolvePRCommentModelName(llm, correlatedLogger); const taskId = job.id || `pr-comment-${pullRequestNumber}-${Date.now()}`; const stateManager = getStateManager(); @@ -430,20 +417,26 @@ export async function processPullRequestCommentJob(job: Job): Pr } const state: ProcessingState = { octokit: null, localRepoPath: undefined, worktreeInfo: undefined, claudeResult: null, authorsText: '', unprocessedComments: [], startingWorkComment: null }; + const activeAttemptRouting: ActiveAttemptRouting = {}; try { // Branch early for review mode — read-only analysis, no commits or pushes - if (job.data.commandMode === 'review') { - return await runWithExecutionAbortSignal(executionController.signal, () => executeReviewProcessing({ job, context, llm, taskId, stateManager, state, redisClient, validatePRAndComments }), hashTaskAttemptToken(lockToken)); + const result = job.data.commandMode === 'review' + ? await runWithExecutionAbortSignal(executionController.signal, () => executeReviewProcessing({ job, context, llm, taskId, stateManager, state, redisClient, validatePRAndComments }), hashTaskAttemptToken(lockToken)) + : await runWithExecutionAbortSignal(executionController.signal, () => executeProcessing({ job, context, llm, taskId, stateManager, state, lockKey, lockToken }, activeAttemptRouting), hashTaskAttemptToken(lockToken)); + if (result.reason === 'provider_limit_retry_superseded') { + await restoreSupersededProviderLimitComments(context, { repoOwner, repoName, pullRequestNumber, redisClient }); } - return await runWithExecutionAbortSignal(executionController.signal, () => executeProcessing({ job, context, llm, taskId, stateManager, state, lockKey, lockToken }), hashTaskAttemptToken(lockToken)); + return result; } catch (error) { - await handleJobError(error as Error, job, { pullRequestNumber, repoOwner, repoName, authorsText: state.authorsText, unprocessedComments: state.unprocessedComments, octokit: state.octokit, startingWorkComment: state.startingWorkComment, claudeResult: state.claudeResult, correlationId, correlatedLogger, stateManager, taskId }); + const errorDisposition = await handleJobError(error as Error, job, { pullRequestNumber, repoOwner, repoName, authorsText: state.authorsText, unprocessedComments: state.unprocessedComments, octokit: state.octokit, startingWorkComment: state.startingWorkComment, claudeResult: state.claudeResult, correlationId, correlatedLogger, stateManager, taskId, redisClient, runtimeAgentAlias: activeAttemptRouting.agentAlias, runtimeModelName: activeAttemptRouting.modelName }); + if (errorDisposition === 'provider_limit_retry_superseded') { + await restoreSupersededProviderLimitComments(context, { repoOwner, repoName, pullRequestNumber, redisClient }); + return { status: 'skipped', reason: 'provider_limit_retry_superseded', pullRequestNumber }; + } // Don't re-throw for user cancellations (not an error, just cancelled) const isUserCancelled = (error as Error).message?.includes('aborted by user'); - if (isUserCancelled) { - return { status: 'cancelled', reason: 'user_cancelled' }; - } + if (isUserCancelled) return { status: 'cancelled', reason: 'user_cancelled' }; if (!(error instanceof UsageLimitError)) throw error; return { status: 'requeued', reason: 'usage_limit' }; } finally { diff --git a/src/shared/slashCommandsBlock.ts b/src/shared/slashCommandsBlock.ts index 809a62b62..75ae7138b 100644 --- a/src/shared/slashCommandsBlock.ts +++ b/src/shared/slashCommandsBlock.ts @@ -13,7 +13,7 @@ export function buildSlashCommandsBlock(): string { '| `/review` | Request an AI code review | `/review` or `/review claude-sonnet` |', '| `/fix` | Implement fixes for issues found by `/review` | `/fix` or `/fix address the null check issue` |', '| `/switch` | Change the AI model for this PR | `/switch claude-opus` |', - '| `/use` | Override the model for a single follow-up run | `/use claude-sonnet` |', + '| `/use` | Switch the PR model and run a follow-up | `/use claude-sonnet` |', '| `/ultrafix` | Loop review→fix cycles until score goal is met | `/ultrafix` or `/ultrafix goal=8 max=10` |', '', '', diff --git a/test/commentEventHandler.switch-use.test.ts b/test/commentEventHandler.switch-use.test.ts index 86d160d88..05dc7be9c 100644 --- a/test/commentEventHandler.switch-use.test.ts +++ b/test/commentEventHandler.switch-use.test.ts @@ -4,8 +4,11 @@ import { createHash } from 'node:crypto'; import type { IssueCommentEvent, Label } from '@octokit/webhooks-types'; import { createWebhookIssueCommentCreatedEvent, createWebhookPRReviewCommentCreatedEvent, createMockLabel } from './testHelpers.js'; +const actualLabelOperations = await import('../packages/core/src/utils/github/labelOperations.js'); + function manualRevisionIdentity(updatedAt: string, body: string, eventType = 'issue_comment'): string { - const digest = createHash('sha256').update(`${eventType}\0${body}`).digest('hex').slice(0, 12); + const commentType = eventType === 'pull_request_review_comment' ? 'review' : 'issue'; + const digest = createHash('sha256').update(`${commentType}\0${body}`).digest('hex').slice(0, 12); return `${updatedAt}:${digest}`; } @@ -148,7 +151,13 @@ await mock.module('../packages/core/src/utils/commentFilters.js', { }); // Mock safeUpdateLabels — capture calls for assertions -const mockSafeUpdateLabels = mock.fn(async () => {}); +const defaultSafeUpdateLabels = async (_context: unknown, removed: string[] = [], added: string[] = []) => ({ + success: true, + removed, + added, + errors: [], +}); +const mockSafeUpdateLabels = mock.fn(defaultSafeUpdateLabels); await mock.module('../packages/core/src/utils/github/labelOperations.js', { namedExports: { safeRemoveLabel: mock.fn(async () => true), @@ -187,6 +196,8 @@ const { shutdownQueue } = await import('../packages/core/src/queue/taskQueue.js' const { applyPendingCommentCommandContext } = await import( '../src/jobs/prPendingComments.js' ); +const { dedupeUnprocessedComments } = await import('../packages/core/src/utils/pendingComments.js'); +const { extractModelFromLabels, handleJobError, UsageLimitError } = await import('../src/jobs/prCommentJobUtils.js'); const mockInvalidateAutomaticWork = mock.fn(async () => ({ workEpoch: 1, hadAutomaticWork: false })); const mockHasAutomaticWork = mock.fn(async () => false); @@ -204,6 +215,7 @@ setUltrafixDeps({ }); beforeEach(() => { + mockSafeUpdateLabels.mock.mockImplementation(defaultSafeUpdateLabels); mockInvalidateAutomaticWork.mock.resetCalls(); mockInvalidateAutomaticWork.mock.mockImplementation(async () => ({ workEpoch: 1, hadAutomaticWork: false })); mockHasAutomaticWork.mock.resetCalls(); @@ -219,6 +231,8 @@ after(async () => { function createMockRedis() { const store = new Map(); + const lists = new Map(); + const counters = new Map(); return { get: mock.fn(async (key: string) => store.get(key) ?? null), setex: mock.fn(async (key: string, _ttl: number, value: string) => { @@ -232,14 +246,43 @@ function createMockRedis() { del: mock.fn(async (key: string) => { store.delete(key); }), - eval: mock.fn(async (_script: string, _keyCount: number, key: string, token: string) => { + incr: mock.fn(async (key: string) => { + const next = (counters.get(key) ?? 0) + 1; + counters.set(key, next); + return next; + }), + lrange: mock.fn(async (key: string) => [...(lists.get(key) ?? [])]), + eval: mock.fn(async (script: string, _keyCount: number, key: string, ...args: string[]) => { + if (script.includes("redis.call('LRANGE'")) { + const list = lists.get(key) ?? []; + const seen = new Set(list.map(raw => { + const comment = JSON.parse(raw) as { type: string; id: number; revisionIdentity?: string; updatedAt?: string; createdAt?: string; body: string }; + const revision = comment.updatedAt ?? comment.createdAt ?? ''; + const digest = createHash('sha256').update(`${comment.type}\0${comment.body}`).digest('hex').slice(0, 12); + return `${comment.type}:${comment.id}:${comment.revisionIdentity ?? `${revision}:${digest}`}`; + })); + const missing: string[] = []; + for (let index = 0; index < args.length; index += 3) { + if (!seen.has(args[index])) { + seen.add(args[index]); + missing.push(args[index + 2]); + } + } + lists.set(key, [...missing, ...list]); + return missing.length; + } + const token = args[0]; if (store.get(key) !== token) return 0; + if (script.includes("redis.call('PEXPIRE'")) return 1; store.delete(key); return 1; }), - rpush: mock.fn(async () => {}), + rpush: mock.fn(async (key: string, ...values: string[]) => { + lists.set(key, [...(lists.get(key) ?? []), ...values]); + }), expire: mock.fn(async () => {}), _store: store, + _lists: lists, }; } @@ -307,7 +350,7 @@ describe('commentEventHandler — /switch command', () => { const call = mockSafeUpdateLabels.mock.calls[0]; const newLabels = call.arguments[2] as string[]; // "opus" should be resolved via the current configured alias. - assert.deepStrictEqual(newLabels, ['llm-claude-opus-5']); + assert.deepStrictEqual(newLabels, ['llm-claude-opus5']); }); test('/switch with full model ID preserves it in label', async () => { @@ -318,7 +361,7 @@ describe('commentEventHandler — /switch command', () => { assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); const newLabels = mockSafeUpdateLabels.mock.calls[0].arguments[2] as string[]; - assert.deepStrictEqual(newLabels, ['llm-claude-sonnet-4-6']); + assert.deepStrictEqual(newLabels, ['llm-claude-sonnet46']); }); test('/switch removes existing LLM labels and adds new one', async () => { @@ -341,7 +384,7 @@ describe('commentEventHandler — /switch command', () => { assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); const [, existingLlmLabels, newLabels] = mockSafeUpdateLabels.mock.calls[0].arguments; assert.deepStrictEqual(existingLlmLabels, ['llm-claude-opus-4-6']); - assert.deepStrictEqual(newLabels, ['llm-claude-sonnet-5']); + assert.deepStrictEqual(newLabels, ['llm-claude-sonnet5']); }); test('/switch without model argument warns and returns early', async () => { @@ -404,7 +447,7 @@ describe('commentEventHandler — /switch command', () => { assert.ok(comments[0].body.includes('Please review the auth module'), 'Comment body should contain the user instructions'); }); - test('/switch with custom MODEL_LABEL_PATTERN uses pattern-derived prefix for new labels', async () => { + test('/switch derives a validated canonical label for a custom MODEL_LABEL_PATTERN', async () => { // Simulate PR with a custom-prefixed model label mockOctokit.request.mock.mockImplementation(async () => ({ data: { @@ -421,10 +464,9 @@ describe('commentEventHandler — /switch command', () => { await processCommentEvent(event, 'issue_comment', 'corr-custom-pattern', config); assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); - const [, existingLlmLabels, newLabels] = mockSafeUpdateLabels.mock.calls[0].arguments; - assert.deepStrictEqual(existingLlmLabels, ['ai-model-claude-opus-4-6']); - // New label should use the custom prefix, not hardcoded 'llm-' - assert.deepStrictEqual(newLabels, ['ai-model-claude-sonnet-5']); + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[1], ['ai-model-claude-opus-4-6']); + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['ai-model-claude-sonnet5']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 0); }); test('/switch with llm- prefixed argument strips prefix before resolving', async () => { @@ -436,7 +478,7 @@ describe('commentEventHandler — /switch command', () => { assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); const newLabels = mockSafeUpdateLabels.mock.calls[0].arguments[2] as string[]; // "llm-haiku" → normalizeModelLabel strips "llm-" → "haiku" → resolveModelAlias → "claude-haiku-4-5-20251001" - assert.deepStrictEqual(newLabels, ['llm-claude-haiku-4-5-20251001']); + assert.deepStrictEqual(newLabels, ['llm-claude-haiku']); }); test('/switch removes multiple existing LLM labels', async () => { @@ -459,10 +501,10 @@ describe('commentEventHandler — /switch command', () => { assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); const [, existingLlmLabels, newLabels] = mockSafeUpdateLabels.mock.calls[0].arguments; assert.deepStrictEqual(existingLlmLabels, ['llm-claude-opus-4-6', 'llm-claude-sonnet-4-6']); - assert.deepStrictEqual(newLabels, ['llm-claude-haiku-4-5-20251001']); + assert.deepStrictEqual(newLabels, ['llm-claude-haiku']); }); - test('/switch works with escaped metacharacters in MODEL_LABEL_PATTERN like ^model\\-(.+)$', async () => { + test('/switch supports a safely derived escaped-pattern label', async () => { // Escaped metacharacters like \- should be handled correctly by modelLabelPrefix, // deriving the literal prefix 'model-' which produces labels matching the pattern. const event = createPRCommentEvent('/switch opus'); @@ -470,10 +512,8 @@ describe('commentEventHandler — /switch command', () => { await processCommentEvent(event, 'issue_comment', 'corr-escaped', config); - // Should call safeUpdateLabels with the derived prefix 'model-' assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); - const newLabels = mockSafeUpdateLabels.mock.calls[0].arguments[2] as string[]; - assert.deepStrictEqual(newLabels, ['model-claude-opus-5']); + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['model-claude-opus5']); }); test('/switch aborts when derived label prefix would not match MODEL_LABEL_PATTERN', async () => { @@ -499,7 +539,7 @@ describe('commentEventHandler — /switch command', () => { // Should still update labels using the first model assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); const newLabels = mockSafeUpdateLabels.mock.calls[0].arguments[2] as string[]; - assert.deepStrictEqual(newLabels, ['llm-claude-opus-5']); + assert.deepStrictEqual(newLabels, ['llm-claude-opus5']); // Should have logged a warning about extra arguments const warnCalls = mockLoggerInstance.warn.mock.calls; const extraWarn = warnCalls.find( @@ -542,16 +582,729 @@ describe('commentEventHandler — /use command', () => { })); }); - test('/use enqueues a job without updating labels', async () => { + test('/use updates the durable model label before enqueueing', async () => { const event = createPRCommentEvent('/use opus'); const config = createTestConfig(); await processCommentEvent(event, 'issue_comment', 'corr-10', config); - // /use should NOT update labels - assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 0); - // /use SHOULD enqueue a job + assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['llm-claude-opus5']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + }); + + test('/use queues the validated label derived from a custom MODEL_LABEL_PATTERN', async () => { + const event = createPRCommentEvent('/use opus\nContinue the fix'); + const config = createTestConfig({ MODEL_LABEL_PATTERN: '^ai-model-(.+)$' }); + + await processCommentEvent(event, 'issue_comment', 'corr-use-custom-pattern', config); + + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['ai-model-claude-opus5']); + const jobData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.strictEqual(jobData.modelLabel, 'ai-model-claude-opus5'); + assert.strictEqual(jobData.modelName, 'claude-opus-5'); + }); + + test('out-of-order /use revisions keep the durable label aligned with the newest queued routing', async () => { + mockActiveJobs = [{ + name: 'processPullRequestComment', + data: { pullRequestNumber: 42, repoOwner: 'testowner', repoName: 'testrepo' }, + }]; + const newer = createPRCommentEvent('/use haiku\nUse the newer revision'); + newer.comment.id = 12345; + newer.comment.created_at = '2026-08-14T10:00:00Z'; + newer.comment.updated_at = '2026-08-14T10:05:00Z'; + const config = createTestConfig({ processCommentEvent }); + + await processCommentEvent(newer, 'issue_comment', 'corr-newer-use-revision', config); + + const queuedData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + mockWaitingJobs = [{ + id: mockQueueAdd.mock.calls[0].arguments[2].jobId, + name: 'processPullRequestComment', + data: queuedData, + remove: mock.fn(async () => {}), + }]; + const older = createPRCommentEvent('/use opus\nUse the older revision'); + older.comment.id = newer.comment.id; + older.comment.created_at = newer.comment.created_at; + older.comment.updated_at = '2026-08-14T10:01:00Z'; + + await handleCommentEdited(older, 'issue_comment', 'corr-older-use-revision', config); + + assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1, 'stale delivery must not mutate the model label'); + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['llm-claude-haiku']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1, 'stale delivery must not enqueue different routing'); + assert.strictEqual(queuedData.agentAlias, 'default'); + assert.strictEqual(queuedData.modelName, 'claude-haiku-4-5-20251001'); + assert.strictEqual(queuedData.modelLabel, 'llm-claude-haiku'); + }); + + test('concurrent older and newer /use commands reject the stale transition after lease acquisition', async () => { + mockSafeUpdateLabels.mock.mockImplementation(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'llm-claude-sonnet5']; + let releaseOlderPullRead!: () => void; + let olderReachedPullRead!: () => void; + const olderPullRead = new Promise(resolve => { olderReachedPullRead = resolve; }); + const olderPullGate = new Promise(resolve => { releaseOlderPullRead = resolve; }); + let pullReads = 0; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + pullReads += 1; + if (pullReads === 1) { + olderReachedPullRead(); + await olderPullGate; + } + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + return { data: {} }; + }); + const config = createTestConfig(); + const older = createPRCommentEvent('/use opus\nOlder command'); + older.comment.id = 100; + older.comment.created_at = '2026-08-14T10:00:00Z'; + older.comment.updated_at = older.comment.created_at; + const newer = createPRCommentEvent('/use haiku\nNewer command'); + newer.comment.id = 101; + newer.comment.created_at = '2026-08-14T10:01:00Z'; + newer.comment.updated_at = newer.comment.created_at; + + const olderProcessing = processCommentEvent(older, 'issue_comment', 'corr-concurrent-older', config); + await olderPullRead; + await processCommentEvent(newer, 'issue_comment', 'corr-concurrent-newer', config); + const claimedRevision = JSON.parse( + config.redisClient._store.get('pr-model-command-revision:testowner:testrepo:42') ?? 'null', + ) as { id: number } | null; + assert.strictEqual(claimedRevision?.id, newer.comment.id); + releaseOlderPullRead(); + await olderProcessing; + const finalClaimedRevision = JSON.parse( + config.redisClient._store.get('pr-model-command-revision:testowner:testrepo:42') ?? 'null', + ) as { id: number; createdAt?: string } | null; + assert.strictEqual(finalClaimedRevision?.id, newer.comment.id, JSON.stringify(finalClaimedRevision)); + + assert.deepStrictEqual(liveLabels, ['AI', 'llm-claude-haiku']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + const queued = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.strictEqual(queued.commandCommentId, newer.comment.id); + assert.strictEqual(queued.modelName, 'claude-haiku-4-5-20251001'); + }); + + test('queue publication stays inside the model transition lease so a newer /use becomes the only executable routing', async () => { + mockSafeUpdateLabels.mock.mockImplementation(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'llm-claude-sonnet5']; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + return { data: {} }; + }); + + const config = createTestConfig(); + let observeLeaseWait!: () => void; + const leaseWaitObserved = new Promise(resolve => { observeLeaseWait = resolve; }); + let leaseWaitSignalled = false; + config.redisClient.set.mock.mockImplementation(async (key: string, value: string, ...args: string[]) => { + if (args.includes('NX') && config.redisClient._store.has(key)) { + if (key.startsWith('ultrafix:label-transition:') && !leaseWaitSignalled) { + leaseWaitSignalled = true; + observeLeaseWait(); + } + return null; + } + config.redisClient._store.set(key, value); + return 'OK'; + }); + + let releaseOlderQueue!: () => void; + let observeOlderQueue!: () => void; + const olderQueueReached = new Promise(resolve => { observeOlderQueue = resolve; }); + const olderQueueGate = new Promise(resolve => { releaseOlderQueue = resolve; }); + const durableJobs: Array<{ id: string; name: string; data: Record; remove: ReturnType }> = []; + mockQueueAdd.mock.mockImplementationOnce(async (_name: string, data: Record, options: { jobId: string }) => { + observeOlderQueue(); + await olderQueueGate; + const job = { id: options.jobId, name: 'processPullRequestComment', data, remove: mock.fn(async () => {}) }; + durableJobs.push(job); + mockWaitingJobs = [...durableJobs]; + }); + + const older = createPRCommentEvent('/use opus\nOlder follow-up'); + older.comment.id = 100; + older.comment.created_at = '2026-08-14T10:00:00Z'; + older.comment.updated_at = older.comment.created_at; + const newer = createPRCommentEvent('/use haiku\nNewer follow-up'); + newer.comment.id = 101; + newer.comment.created_at = '2026-08-14T10:01:00Z'; + newer.comment.updated_at = newer.comment.created_at; + + const olderProcessing = processCommentEvent(older, 'issue_comment', 'corr-publication-older', config); + await olderQueueReached; + let newerCompleted = false; + const newerProcessing = processCommentEvent(newer, 'issue_comment', 'corr-publication-newer', config) + .then(() => { newerCompleted = true; }); + await leaseWaitObserved; + assert.strictEqual(newerCompleted, false, 'newer delivery must wait while the older queue publication owns the PR lease'); + + releaseOlderQueue(); + await Promise.all([olderProcessing, newerProcessing]); + + assert.deepStrictEqual(liveLabels, ['AI', 'llm-claude-haiku']); + assert.strictEqual(durableJobs.length, 1, 'serialized publication must not create a second explicit job'); + const pending = (config.redisClient._lists.get('pending-pr-comments:testowner:testrepo:42') ?? []) + .map(raw => JSON.parse(raw)); + assert.deepStrictEqual(pending.map(comment => comment.id), [newer.comment.id]); + + const executableData = { ...durableJobs[0].data }; + const executableComments = [ + ...((durableJobs[0].data.comments as unknown[]) ?? []), + ...pending, + ]; + const commentsForExecution = applyPendingCommentCommandContext( + executableData, + executableComments, + mockLoggerInstance as never, + ); + assert.strictEqual(executableData.agentAlias, 'default'); + assert.strictEqual(executableData.modelName, 'claude-haiku-4-5-20251001'); + assert.strictEqual(executableData.modelLabel, 'llm-claude-haiku'); + assert.deepStrictEqual(commentsForExecution.map(comment => comment.id), [older.comment.id, newer.comment.id]); + assert.deepStrictEqual( + (executableData.comments as Array<{ id: number }>).map(comment => comment.id), + [older.comment.id, newer.comment.id], + 'both comments must remain durable exactly once', + ); + }); + + test('same-timestamp edits that only change /use routing keep distinct original-body revisions', async () => { + mockActiveJobs = [{ + name: 'processPullRequestComment', + data: { pullRequestNumber: 42, repoOwner: 'testowner', repoName: 'testrepo' }, + }]; + const original = createPRCommentEvent('/use opus\nFix this'); + original.comment.id = 12345; + original.comment.updated_at = '2026-08-14T10:00:00Z'; + const config = createTestConfig({ processCommentEvent }); + + await processCommentEvent(original, 'issue_comment', 'corr-use-original-routing', config); + const edited = createPRCommentEvent('/use haiku\nFix this'); + edited.comment.id = original.comment.id; + edited.comment.created_at = original.comment.created_at; + edited.comment.updated_at = original.comment.updated_at; + await handleCommentEdited(edited, 'issue_comment', 'corr-use-edited-routing', config); + + const pending = [...config.redisClient._lists.values()].flat().map((raw: string) => JSON.parse(raw)); + assert.strictEqual(pending.length, 2); + assert.deepStrictEqual(pending.map((comment: { body: string }) => comment.body), ['Fix this', 'Fix this']); + assert.deepStrictEqual( + pending.map((comment: { revisionIdentity: string }) => comment.revisionIdentity), + [ + manualRevisionIdentity(original.comment.updated_at, original.comment.body), + manualRevisionIdentity(edited.comment.updated_at, edited.comment.body), + ], + ); + assert.strictEqual(dedupeUnprocessedComments(pending).length, 2); + assert.deepStrictEqual(pending.map((comment: { modelName: string }) => comment.modelName), [ + 'claude-opus-5', + 'claude-haiku-4-5-20251001', + ]); + }); + + test('issue-comment transition preserves unrelated labels added and removed between live reads and writes', async () => { + mockSafeUpdateLabels.mock.mockImplementationOnce(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'security', 'workflow-ready', 'llm-claude-opus48']; + let issueReads = 0; + const endpoints: string[] = []; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + endpoints.push(endpoint); + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + issueReads += 1; + const snapshot = [...liveLabels]; + if (issueReads === 1) { + liveLabels.push('release-blocker'); + liveLabels.splice(liveLabels.indexOf('workflow-ready'), 1); + } + return { data: { labels: snapshot.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + return { data: {} }; + }); + + await processCommentEvent(createPRCommentEvent('/use opus'), 'issue_comment', 'corr-live-label-race', createTestConfig()); + + assert.deepStrictEqual(liveLabels, ['AI', 'security', 'release-blocker', 'llm-claude-opus5']); + assert.ok(!endpoints.some(endpoint => endpoint.startsWith('PUT ')), 'model transition must not replace the complete label set'); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + }); + + test('stale review-comment payload uses live labels and retries a concurrent competing model label', async () => { + mockSafeUpdateLabels.mock.mockImplementationOnce(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'release-blocker', 'llm-claude-opus48']; + let issueReads = 0; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + issueReads += 1; + if (issueReads === 2) liveLabels.push('llm-claude-haiku'); + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + return { data: {} }; + }); + const event = createPRReviewCommentEvent('/use opus'); + event.pull_request.labels = [{ name: 'AI' }, { name: 'llm-claude-sonnet46' }] as typeof event.pull_request.labels; + + await processCommentEvent(event, 'pull_request_review_comment', 'corr-stale-review-labels', createTestConfig()); + + assert.deepStrictEqual(liveLabels, ['AI', 'release-blocker', 'llm-claude-opus5']); + assert.strictEqual(issueReads, 4, 'verification conflict should force a second live read/mutate/verify attempt'); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + }); + + test('/use does not enqueue or acknowledge when live model-label convergence cannot be verified', async () => { + mockSafeUpdateLabels.mock.mockImplementationOnce(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'release-blocker', 'llm-claude-opus48']; + let issueReads = 0; + let acknowledgements = 0; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + issueReads += 1; + if (issueReads % 2 === 0 && !liveLabels.includes('llm-claude-haiku')) liveLabels.push('llm-claude-haiku'); + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments') acknowledgements += 1; + return { data: {} }; + }); + + await processCommentEvent(createPRCommentEvent('/use opus'), 'issue_comment', 'corr-nonconvergent-labels', createTestConfig()); + + assert.strictEqual(issueReads, 9); + assert.strictEqual(mockQueueAdd.mock.callCount(), 0); + assert.strictEqual(acknowledgements, 0); + assert.ok(liveLabels.includes('release-blocker')); + assert.ok(liveLabels.includes('llm-claude-opus48')); + assert.ok(!liveLabels.includes('llm-claude-opus5')); + }); + + for (const leaseFailure of ['acquisition', 'ownership'] as const) { + test(`/use surfaces retryable failure without enqueue or acknowledgement when label-transition lease ${leaseFailure} fails`, async () => { + mockSafeUpdateLabels.mock.mockImplementationOnce(actualLabelOperations.safeUpdateLabels); + const liveLabels = ['AI', 'llm-claude-opus48']; + let acknowledgements = 0; + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments') acknowledgements += 1; + return { data: {} }; + }); + const config = createTestConfig(); + if (leaseFailure === 'acquisition') { + config.redisClient.set.mock.mockImplementation(async (key: string, value: string, ...args: string[]) => { + if (key.startsWith('ultrafix:label-transition:')) throw new Error('redis unavailable'); + if (args.includes('NX') && config.redisClient._store.has(key)) return null; + config.redisClient._store.set(key, value); + return 'OK'; + }); + } else { + config.redisClient.eval.mock.mockImplementation(async () => 0); + } + + const event = createPRCommentEvent('/use opus'); + await assert.rejects( + processCommentEvent(event, 'issue_comment', `corr-lease-${leaseFailure}`, config), + { name: 'LabelTransitionLeaseError' }, + ); + + assert.strictEqual(mockQueueAdd.mock.callCount(), 0); + assert.strictEqual(acknowledgements, 0); + assert.strictEqual( + config.redisClient._store.has(`pr-comment-processed:testowner:testrepo:42:${event.comment.id}`), + false, + 'failed delivery must release its exact slash-command claim', + ); + }); + } + + test('identical /use redelivery retries after transient lease acquisition failure and enqueues exactly once', async () => { + const event = createPRCommentEvent('/use opus\nRetry this command'); + const config = createTestConfig(); + let leaseAcquisitions = 0; + let acknowledgements = 0; + config.redisClient.set.mock.mockImplementation(async (key: string, value: string, ...args: string[]) => { + if (key.startsWith('ultrafix:label-transition:')) { + leaseAcquisitions += 1; + if (leaseAcquisitions === 1) throw new Error('transient redis failure'); + } + if (args.includes('NX') && config.redisClient._store.has(key)) return null; + config.redisClient._store.set(key, value); + return 'OK'; + }); + mockOctokit.request.mock.mockImplementation(async (endpoint: string) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: [] } }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments') acknowledgements += 1; + return { data: {} }; + }); + + await assert.rejects( + processCommentEvent(event, 'issue_comment', 'corr-transient-lease-first', config), + { name: 'LabelTransitionLeaseError' }, + ); + const redelivery = await processCommentEvent(event, 'issue_comment', 'corr-transient-lease-redelivery', config); + + assert.deepStrictEqual(redelivery, { + status: 'accepted', + billing: { seatConsumed: true }, + evidence: { triggerCommentIds: [event.comment.id] }, + }); + assert.strictEqual(mockSafeUpdateLabels.mock.callCount(), 1); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + assert.strictEqual(acknowledgements, 1); + assert.strictEqual( + mockQueueAdd.mock.calls[0].arguments[2].jobId, + `pr-comments-batch-testowner-testrepo-42-${event.comment.id}-${manualRevisionSlug(event.comment.updated_at, event.comment.body)}`, + ); + }); + + test('ownership loss after durable /use publication retries without duplicating its revision, job, or acknowledgement', async () => { + mockSafeUpdateLabels.mock.mockImplementation(actualLabelOperations.safeUpdateLabels); + const event = createPRCommentEvent('/use opus\nRetry published work'); + const config = createTestConfig(); + const liveLabels = ['AI', 'llm-claude-opus48']; + let acknowledgements = 0; + let jobPublished = false; + let ownershipFailureInjected = false; + + mockOctokit.request.mock.mockImplementation(async (endpoint: string, options: Record) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + return { data: { head: { ref: 'feature-branch' }, labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'GET /repos/{owner}/{repo}/issues/{issue_number}') { + return { data: { labels: liveLabels.map(name => ({ name })) } }; + } + if (endpoint === 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}') { + const index = liveLabels.findIndex(name => name.toLowerCase() === String(options.name).toLowerCase()); + if (index >= 0) liveLabels.splice(index, 1); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels') { + for (const name of options.labels as string[]) if (!liveLabels.includes(name)) liveLabels.push(name); + return { data: {} }; + } + if (endpoint === 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments') acknowledgements += 1; + return { data: {} }; + }); + mockQueueAdd.mock.mockImplementationOnce(async (name: string, data: Record, options: { jobId: string }) => { + jobPublished = true; + mockWaitingJobs = [{ id: options.jobId, name, data, remove: mock.fn(async () => {}) }]; + }); + config.redisClient.eval.mock.mockImplementation(async (script: string, _keyCount: number, key: string, ...args: string[]) => { + if (script.includes("redis.call('PEXPIRE'") && jobPublished && !ownershipFailureInjected) { + ownershipFailureInjected = true; + config.redisClient._store.delete(key); + return 0; + } + const token = args[0]; + if (config.redisClient._store.get(key) !== token) return 0; + if (script.includes("redis.call('PEXPIRE'")) return 1; + config.redisClient._store.delete(key); + return 1; + }); + + await assert.rejects( + processCommentEvent(event, 'issue_comment', 'corr-published-lease-first', config), + { name: 'LabelTransitionLeaseError' }, + ); + assert.deepStrictEqual(liveLabels, ['AI', 'llm-claude-opus5']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + assert.strictEqual(acknowledgements, 0); + + const redelivery = await processCommentEvent(event, 'issue_comment', 'corr-published-lease-redelivery', config); + const duplicate = await processCommentEvent(event, 'issue_comment', 'corr-published-lease-duplicate', config); + + assert.strictEqual(redelivery.status, 'accepted'); + assert.deepStrictEqual(duplicate, { status: 'ignored', reason: 'duplicate_delivery' }); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1, 'the deterministic published job must be reused'); + assert.strictEqual(config.redisClient.rpush.mock.callCount(), 0, 'the published revision must not also be copied to pending storage'); + assert.strictEqual(acknowledgements, 1); + const publishedData = mockWaitingJobs[0] as { data: { comments: Array<{ id: number; revisionIdentity: string }> } }; + assert.deepStrictEqual( + publishedData.data.comments.map(comment => ({ id: comment.id, revisionIdentity: comment.revisionIdentity })), + [{ id: event.comment.id, revisionIdentity: manualRevisionIdentity(event.comment.updated_at, event.comment.body) }], + ); + }); + + test('/use persists canonical label, configured agent, and model on the queued job', async () => { + const event = createPRCommentEvent('/use llm-claude-opus5\nContinue with the selected model'); + const config = createTestConfig(); + + await processCommentEvent(event, 'issue_comment', 'corr-canonical-codex', config); + + assert.deepStrictEqual(mockSafeUpdateLabels.mock.calls[0].arguments[2], ['llm-claude-opus5']); + const jobData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.strictEqual(jobData.agentAlias, 'default'); + assert.strictEqual(jobData.modelName, 'claude-opus-5'); + assert.strictEqual(jobData.modelLabel, 'llm-claude-opus5'); + assert.strictEqual(jobData.llm, 'claude-opus-5'); + }); + + test('/use does not queue or acknowledge when the label transition fails', async () => { + mockOctokit.request.mock.mockImplementation(async () => ({ + data: { + head: { ref: 'feature-branch' }, + labels: [{ id: 1, name: 'llm-claude-opus48', color: '000', default: false, description: null, node_id: 'L_1', url: '' }], + }, + })); + mockSafeUpdateLabels.mock.mockImplementationOnce(async () => ({ + success: false, + removed: [], + added: [], + errors: ['failed'], + })); + + await processCommentEvent(createPRCommentEvent('/use sonnet'), 'issue_comment', 'corr-label-failure', createTestConfig()); + + assert.strictEqual(mockQueueAdd.mock.callCount(), 0); + assert.strictEqual(mockOctokit.request.mock.callCount(), 1, 'only the PR read should occur; no success acknowledgement'); + }); + + test('/use supersedes a delayed provider-limit retry and runs the replacement immediately', async () => { + const removeRetry = mock.fn(async () => {}); + mockDelayedJobs = [{ + id: 'pr-comments-batch-testowner-testrepo-42-claude-opus48-main-ratelimit-retry', + name: 'processPullRequestComment', + data: { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + llm: 'claude-opus-4-8', + isRetryFromRateLimit: true, + comments: [{ id: 7, createdAt: '2026-08-14T10:00:00Z', body: 'Original request', author: 'alice', type: 'issue' }], + }, + remove: removeRetry, + }]; + const config = createTestConfig(); + + const event = createPRCommentEvent('/use opus'); + await processCommentEvent(event, 'issue_comment', 'corr-replace-retry', config); + + assert.strictEqual(removeRetry.mock.callCount(), 1); + const restored = config.redisClient._lists.get('pending-pr-comments:testowner:testrepo:42')!.map(raw => JSON.parse(raw) as { id: number }); + assert.deepStrictEqual(restored.map(comment => comment.id), [7, event.comment.id], 'old and selected comments should both remain claimable in order'); assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + const jobData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.strictEqual(jobData.agentAlias, 'default'); + assert.strictEqual(jobData.modelName, 'claude-opus-5'); + assert.strictEqual(mockQueueAdd.mock.calls[0].arguments[2].delay, 3000); + }); + + test('/use queue snapshot cannot race ahead of an old-provider retry publication', async () => { + const config = createTestConfig(); + let retryReadStarted!: () => void; + let usePrReadStarted!: () => void; + let releaseRetryRead!: () => void; + const retryRead = new Promise(resolve => { retryReadStarted = resolve; }); + const usePrRead = new Promise(resolve => { usePrReadStarted = resolve; }); + const retryReadGate = new Promise(resolve => { releaseRetryRead = resolve; }); + let pullReads = 0; + mockOctokit.request.mock.mockImplementation(async (endpoint: string) => { + if (endpoint === 'GET /repos/{owner}/{repo}/pulls/{pull_number}') { + pullReads += 1; + if (pullReads === 1) { + retryReadStarted(); + await retryReadGate; + } else { + usePrReadStarted(); + } + return { + data: { + head: { ref: 'feature-branch' }, + labels: [{ name: 'llm-claude-opus48' }], + }, + }; + } + return { data: {} }; + }); + + const removeRetry = mock.fn(async () => { mockDelayedJobs = []; }); + mockQueueAdd.mock.mockImplementationOnce(async (name: string, data: Record, queueOptions: Record) => { + mockDelayedJobs = [{ + id: queueOptions.jobId, + name, + data, + remove: removeRetry, + }]; + }); + + const usageLimitHandling = handleJobError( + new UsageLimitError('usage limit', Math.floor(Date.now() / 1000) + 3600), + { + name: 'processPullRequestComment', + data: { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + branchName: 'feature-branch', + comments: [{ id: 7, body: 'Original request', author: 'alice', type: 'issue' }], + llm: 'claude-opus-4-8', + }, + } as never, + { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + authorsText: '@alice', + unprocessedComments: [], + octokit: mockOctokit as never, + startingWorkComment: null, + claudeResult: null, + correlationId: 'corr-retry-use-race', + correlatedLogger: mockLoggerInstance as never, + stateManager: { getTaskState: async () => null } as never, + taskId: 'retry-use-race', + redisClient: config.redisClient as never, + runtimeAgentAlias: 'default', + runtimeModelName: 'claude-opus-4-8', + }, + ); + await retryRead; + + const event = createPRCommentEvent('/use opus'); + const useHandling = processCommentEvent(event, 'issue_comment', 'corr-use-retry-race', config); + await usePrRead; + releaseRetryRead(); + + await Promise.all([usageLimitHandling, useHandling]); + + assert.strictEqual(removeRetry.mock.callCount(), 1, 'the retry published first must be visible to the /use snapshot'); + assert.strictEqual(mockDelayedJobs.length, 0); + assert.strictEqual(mockQueueAdd.mock.callCount(), 2); + assert.ok(String(mockQueueAdd.mock.calls[0].arguments[2].jobId).endsWith('-ratelimit-retry')); + const replacementData = mockQueueAdd.mock.calls[1].arguments[1] as Record; + assert.strictEqual(replacementData.modelName, 'claude-opus-5'); + }); + + test('/use removal failure leaves old and selected comments recoverable without enqueueing a second writer', async () => { + const removalError = new Error('retry became active'); + const removeRetry = mock.fn(async () => { throw removalError; }); + mockDelayedJobs = [{ + id: 'pr-comments-batch-testowner-testrepo-42-old-main-ratelimit-retry', + name: 'processPullRequestComment', + data: { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + isRetryFromRateLimit: true, + comments: [{ id: 7, updatedAt: '2026-08-14T10:01:00Z', body: 'Original request', author: 'alice', type: 'issue' }], + }, + remove: removeRetry, + }]; + const config = createTestConfig(); + + const event = createPRCommentEvent('/use opus'); + await assert.rejects( + processCommentEvent(event, 'issue_comment', 'corr-remove-race', config), + removalError, + ); + + const pending = config.redisClient._lists.get('pending-pr-comments:testowner:testrepo:42')!.map(raw => JSON.parse(raw) as { id: number }); + assert.deepStrictEqual(pending.map(comment => comment.id), [7, event.comment.id]); + assert.strictEqual(mockQueueAdd.mock.callCount(), 0, 'failed removal must not create an overlapping replacement writer'); + }); + + test('/use enqueue failure after retry removal leaves all comments recoverable', async () => { + const enqueueError = new Error('queue unavailable'); + mockQueueAdd.mock.mockImplementationOnce(async () => { throw enqueueError; }); + mockDelayedJobs = [{ + id: 'pr-comments-batch-testowner-testrepo-42-old-main-ratelimit-retry', + name: 'processPullRequestComment', + data: { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + isRetryFromRateLimit: true, + comments: [{ id: 7, body: 'Original request', author: 'alice', type: 'issue' }], + }, + remove: mock.fn(async () => {}), + }]; + const config = createTestConfig(); + + const event = createPRCommentEvent('/use opus'); + await assert.rejects( + processCommentEvent(event, 'issue_comment', 'corr-enqueue-race', config), + enqueueError, + ); + + const pending = config.redisClient._lists.get('pending-pr-comments:testowner:testrepo:42')!.map(raw => JSON.parse(raw) as { id: number }); + assert.deepStrictEqual(pending.map(comment => comment.id), [7, event.comment.id]); }); test('/use sets commandMode to "use" in job data', async () => { @@ -611,10 +1364,13 @@ describe('commentEventHandler — /use command', () => { assert.strictEqual(mockQueueAdd.mock.callCount(), 1); const jobData = mockQueueAdd.mock.calls[0].arguments[1] as Record; - const comments = jobData.comments as Array<{ body: string }>; + const comments = jobData.comments as Array<{ body: string; revisionIdentity?: string }>; // /use body is stripped like /switch — only user instructions remain assert.ok(comments.length > 0); assert.strictEqual(comments[0].body, 'Refactor the utils'); + const expectedRevision = manualRevisionIdentity(event.comment.updated_at, event.comment.body); + assert.strictEqual(comments[0].revisionIdentity, expectedRevision); + assert.strictEqual(jobData.commandCommentRevisionIdentity, expectedRevision); }); test('/use with llm- prefixed argument strips prefix before resolving', async () => { @@ -711,7 +1467,7 @@ describe('commentEventHandler — commandMode serialization in job data', () => assert.strictEqual(jobData.commandMode, 'switch'); const meta = jobData.commandMeta as { mode: string; models: string[]; instructions: string }; assert.strictEqual(meta.mode, 'switch'); - assert.deepStrictEqual(meta.models, ['sonnet']); + assert.deepStrictEqual(meta.models, ['claude-sonnet-5']); assert.strictEqual(meta.instructions, 'Do a review'); assert.strictEqual(jobData.commandInstructions, 'Do a review'); }); @@ -727,7 +1483,7 @@ describe('commentEventHandler — commandMode serialization in job data', () => assert.strictEqual(jobData.commandMode, 'use'); const meta = jobData.commandMeta as { mode: string; models: string[]; instructions: string }; assert.strictEqual(meta.mode, 'use'); - assert.deepStrictEqual(meta.models, ['haiku']); + assert.deepStrictEqual(meta.models, ['claude-haiku-4-5-20251001']); assert.strictEqual(jobData.commandInstructions, 'Summarize changes'); // LLM should be resolved from /use command assert.strictEqual(jobData.llm, 'claude-haiku-4-5-20251001'); @@ -809,6 +1565,47 @@ describe('commentEventHandler — commandMode serialization in job data', () => }); }); +describe('commentEventHandler — routing provenance', () => { + beforeEach(() => { + mockQueueAdd.mock.resetCalls(); + mockOctokit.request.mock.resetCalls(); + mockActiveJobs = []; + mockWaitingJobs = []; + mockDelayedJobs = []; + }); + + test('ordinary jobs omit explicit provenance and follow the live PR label at execution', async () => { + mockOctokit.request.mock.mockImplementation(async () => ({ + data: { + head: { ref: 'feature-branch' }, + labels: [{ name: 'AI' }, { name: 'llm-codex-gpt55' }], + }, + })); + + await processCommentEvent( + createPRCommentEvent('Please apply the follow-up'), + 'issue_comment', + 'corr-ordinary-live-routing', + createTestConfig(), + ); + + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + const jobData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.strictEqual(jobData.llm, 'codex-gpt55', 'enqueue may retain label A as its ordinary fallback'); + assert.ok(!Object.hasOwn(jobData, 'agentAlias')); + assert.ok(!Object.hasOwn(jobData, 'modelName')); + assert.ok(!Object.hasOwn(jobData, 'modelLabel')); + + const workerLlm = extractModelFromLabels( + [{ name: 'AI' }, { name: 'llm-codex-gpt56-sol' }], + jobData.llm as string, + 42, + mockLoggerInstance as never, + ); + assert.strictEqual(workerLlm, 'codex-gpt56-sol', 'worker validation must prefer live label B over fallback A'); + }); +}); + describe('commentEventHandler — slash command dedup protection', () => { beforeEach(() => { mockSafeUpdateLabels.mock.resetCalls(); @@ -888,7 +1685,7 @@ describe('commentEventHandler — slash command batching/concurrency guard', () })); }); - test('/use is batched when an existing job is active for the same PR', async () => { + test('/use stores its revision and queues a deterministic successor when the active writer finishes at the handoff', async () => { // Simulate an active job for PR 42 mockActiveJobs = [{ name: 'processPullRequestComment', @@ -897,18 +1694,43 @@ describe('commentEventHandler — slash command batching/concurrency guard', () const event = createPRCommentEvent('/use opus\nFix the bug'); const config = createTestConfig(); + const handoffSteps: string[] = []; + mockInvalidateAutomaticWork.mock.mockImplementationOnce(async () => { + handoffSteps.push('invalidate'); + return { workEpoch: 2, hadAutomaticWork: true }; + }); + config.redisClient.rpush.mock.mockImplementationOnce(async (key: string, ...values: string[]) => { + handoffSteps.push('store'); + config.redisClient._lists.set(key, [...(config.redisClient._lists.get(key) ?? []), ...values]); + // The worker crosses its cleanup boundary after the queue snapshot + // but before the pending write completes. + mockActiveJobs = []; + }); + mockQueueAdd.mock.mockImplementationOnce(async () => { handoffSteps.push('enqueue'); }); await processCommentEvent(event, 'issue_comment', 'corr-batch-1', config); - // Should NOT enqueue a new job - assert.strictEqual(mockQueueAdd.mock.callCount(), 0); - // Should store comment for batch via rpush + assert.deepStrictEqual(handoffSteps, ['invalidate', 'store', 'enqueue']); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); assert.strictEqual(config.redisClient.rpush.mock.callCount(), 1); const pendingComment = JSON.parse(config.redisClient.rpush.mock.calls[0].arguments[1] as string) as Record; assert.strictEqual(pendingComment.body, 'Fix the bug'); assert.strictEqual(pendingComment.commandMode, 'use'); assert.strictEqual(pendingComment.commandInstructions, 'Fix the bug'); assert.strictEqual(pendingComment.llmOverride, 'claude-opus-5'); + assert.strictEqual(pendingComment.agentAlias, 'default'); + assert.strictEqual(pendingComment.modelName, 'claude-opus-5'); + assert.strictEqual(pendingComment.modelLabel, 'llm-claude-opus5'); + const successorData = mockQueueAdd.mock.calls[0].arguments[1] as Record; + assert.deepStrictEqual(successorData.comments, [], 'successor must claim the durable pending revision'); + assert.strictEqual( + mockQueueAdd.mock.calls[0].arguments[2].jobId, + `pr-comments-batch-testowner-testrepo-42-${event.comment.id}-${manualRevisionSlug(event.comment.updated_at, event.comment.body)}`, + ); + assert.deepStrictEqual( + mockInvalidateAutomaticWork.mock.calls[0].arguments[1], + { owner: 'testowner', repo: 'testrepo', pr: 42, sourceCommentId: event.comment.id, sourceCommentRevision: manualRevisionIdentity(event.comment.updated_at, event.comment.body) }, + ); }); test('/switch with instructions is batched when an existing job is active', async () => { @@ -943,6 +1765,35 @@ describe('commentEventHandler — slash command batching/concurrency guard', () assert.strictEqual(mockQueueAdd.mock.callCount(), 1); assert.strictEqual(config.redisClient.rpush.mock.callCount(), 0); + assert.strictEqual(mockInvalidateAutomaticWork.mock.callCount(), 1); + assert.deepStrictEqual( + mockInvalidateAutomaticWork.mock.calls[0].arguments[1], + { owner: 'testowner', repo: 'testrepo', pr: 42, sourceCommentId: event.comment.id, sourceCommentRevision: manualRevisionIdentity(event.comment.updated_at, event.comment.body) }, + ); + }); + + test('/use enqueues an independent revision job after fencing queued automatic work', async () => { + mockWaitingJobs = [{ + name: 'processPullRequestComment', + data: { + pullRequestNumber: 42, + repoOwner: 'testowner', + repoName: 'testrepo', + ultrafixMeta: { mode: 'ultrafix', instructions: '', workEpoch: 0 }, + }, + }]; + mockInvalidateAutomaticWork.mock.mockImplementationOnce(async () => ({ workEpoch: 1, hadAutomaticWork: true })); + const event = createPRCommentEvent('/use opus'); + const config = createTestConfig(); + + await processCommentEvent(event, 'issue_comment', 'corr-use-automatic-takeover', config); + + assert.strictEqual(config.redisClient.rpush.mock.callCount(), 0); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + assert.strictEqual( + mockQueueAdd.mock.calls[0].arguments[2].jobId, + `pr-comments-batch-testowner-testrepo-42-${event.comment.id}-${manualRevisionSlug(event.comment.updated_at, event.comment.body)}`, + ); }); test('/review is batched when a waiting job exists for the same PR', async () => { @@ -1167,7 +2018,8 @@ describe('commentEventHandler — slash command batching/concurrency guard', () await processCommentEvent(event, 'pull_request_review_comment', 'corr-batch-review', config); - assert.strictEqual(mockQueueAdd.mock.callCount(), 0); + assert.strictEqual(mockQueueAdd.mock.callCount(), 1); + assert.deepStrictEqual(mockQueueAdd.mock.calls[0].arguments[1].comments, []); assert.strictEqual(config.redisClient.rpush.mock.callCount(), 1); const pendingComment = JSON.parse(config.redisClient.rpush.mock.calls[0].arguments[1] as string) as Record; assert.strictEqual(pendingComment.type, 'review'); @@ -1312,6 +2164,9 @@ describe('applyPendingCommentCommandContext', () => { author: 'alice', type: 'issue' as const, llmOverride: 'claude-opus-4-6', + agentAlias: 'claude-prod', + modelName: 'claude-opus-4-6', + modelLabel: 'production-opus', }, { id: 200, @@ -1355,6 +2210,9 @@ describe('applyPendingCommentCommandContext', () => { ); assert.strictEqual(jobData.llm, 'claude-opus-4-6', `permutation ${permutation.join(',')}`); + assert.strictEqual(jobData.agentAlias, 'claude-prod', `permutation ${permutation.join(',')}`); + assert.strictEqual(jobData.modelName, 'claude-opus-4-6', `permutation ${permutation.join(',')}`); + assert.strictEqual(jobData.modelLabel, 'production-opus', `permutation ${permutation.join(',')}`); } }); diff --git a/test/labelOperations.atomic.test.ts b/test/labelOperations.atomic.test.ts new file mode 100644 index 000000000..5dd4256f3 --- /dev/null +++ b/test/labelOperations.atomic.test.ts @@ -0,0 +1,452 @@ +import { test, mock } from 'node:test'; +import assert from 'node:assert'; +import { safeUpdateLabels } from '../packages/core/src/utils/github/labelOperations.js'; + +const logger = { + debug: mock.fn(), + info: mock.fn(), + warn: mock.fn(), +} as never; + +function createTransitionRedis() { + const store = new Map(); + return { + async set(key: string, value: string, _mode: string, _ttl: number, condition: string) { + if (condition === 'NX' && store.has(key)) return null; + store.set(key, value); + return 'OK'; + }, + async eval(script: string, _keyCount: number, key: string, token: string) { + if (store.get(key) !== token) return 0; + if (script.includes("redis.call('PEXPIRE'")) return 1; + store.delete(key); + return 1; + }, + }; +} + +test('safeUpdateLabels atomically replaces a known current label set', async () => { + const request = mock.fn(async () => ({})); + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + ['llm-claude-opus48'], + ['llm-codex-gpt56-sol'], + ['AI', 'bug', 'llm-claude-opus48'], + ); + + assert.strictEqual(request.mock.callCount(), 1); + assert.strictEqual(request.mock.calls[0].arguments[0], 'PUT /repos/{owner}/{repo}/issues/{issue_number}/labels'); + assert.deepStrictEqual(request.mock.calls[0].arguments[1].labels, ['AI', 'bug', 'llm-codex-gpt56-sol']); + assert.strictEqual(result.success, true); +}); + +test('safeUpdateLabels reports an atomic replacement failure without partial calls', async () => { + const request = mock.fn(async () => { throw new Error('label update denied'); }); + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + ['llm-claude-opus48'], + ['llm-codex-gpt56-sol'], + ['AI', 'llm-claude-opus48'], + ); + + assert.strictEqual(request.mock.callCount(), 1); + assert.strictEqual(result.success, false); + assert.deepStrictEqual(result.removed, []); + assert.deepStrictEqual(result.added, []); +}); + +test('exclusive convergence restores the prior model label when a later target addition fails', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + let targetAddAttempts = 0; + let oldLabelDeleted = false; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) { + return { data: { labels: [...labels] } }; + } + if (endpoint.startsWith('POST ')) { + const [label] = options.labels as string[]; + if (label === 'llm-codex-gpt56-sol') { + targetAddAttempts += 1; + if (targetAddAttempts > 1) throw new Error('target label unavailable'); + } + labels.add(label); + return {}; + } + if (endpoint.startsWith('DELETE ')) { + const label = options.name as string; + labels.delete(label); + if (label === 'llm-claude-opus48') { + oldLabelDeleted = true; + // Simulate the established target being concurrently removed, + // forcing the next convergence attempt to add it again. + labels.delete('llm-codex-gpt56-sol'); + } + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + ['llm-claude-opus48'], + ['llm-codex-gpt56-sol'], + { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 2, + redis: createTransitionRedis() as never, + }, + ); + + assert.strictEqual(oldLabelDeleted, true); + assert.strictEqual(targetAddAttempts, 2); + assert.strictEqual(result.success, false); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-claude-opus48']); + assert.deepStrictEqual(result.finalLabels?.sort(), ['AI', 'llm-claude-opus48']); +}); + +test('exclusive convergence does not restore when the initial model-label snapshot fails', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + let issueReads = 0; + const mutations: string[] = []; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) { + issueReads += 1; + if (issueReads === 1) throw new Error('initial labels unavailable'); + return { data: { labels: [...labels] } }; + } + if (endpoint.startsWith('DELETE ')) { + mutations.push(endpoint); + labels.delete(options.name as string); + return {}; + } + if (endpoint.startsWith('POST ')) { + mutations.push(endpoint); + labels.add((options.labels as string[])[0]); + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + ['llm-claude-opus48'], + ['llm-codex-gpt56-sol'], + { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: createTransitionRedis() as never, + }, + ); + + assert.strictEqual(result.success, false); + assert.strictEqual(issueReads, 1); + assert.deepStrictEqual(mutations, []); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-claude-opus48']); +}); + +test('exclusive convergence removes an introduced target when verification fails with no prior model label', async () => { + const labels = new Set(['AI']); + let issueReads = 0; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) { + issueReads += 1; + if (issueReads === 2) throw new Error('verification unavailable'); + return { data: { labels: [...labels] } }; + } + if (endpoint.startsWith('POST ')) { + labels.add((options.labels as string[])[0]); + return {}; + } + if (endpoint.startsWith('DELETE ')) { + labels.delete(options.name as string); + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + [], + ['llm-codex-gpt56-sol'], + { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: createTransitionRedis() as never, + }, + ); + + assert.strictEqual(result.success, false); + assert.deepStrictEqual([...labels], ['AI']); + assert.deepStrictEqual(result.finalLabels, ['AI']); +}); + +test('failed transition rollback preserves a newer verified singleton when B starts before rollback', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + const redis = createTransitionRedis(); + let runningNewerTransition = false; + let startedNewerTransition = false; + let newerPromise: Promise>> | undefined; + let newerResult: Awaited> | undefined; + const context = { octokit: { request: undefined as never }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) { + if (!runningNewerTransition && !startedNewerTransition && labels.has('llm-codex-gpt56-sol')) { + startedNewerTransition = true; + runningNewerTransition = true; + newerPromise = safeUpdateLabels(context, [], [], { + targetLabel: 'llm-gemini-3-pro', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: redis as never, + }).then(result => { + newerResult = result; + runningNewerTransition = false; + return result; + }); + await Promise.resolve(); + throw new Error('older transition verification failed'); + } + return { data: { labels: [...labels] } }; + } + if (endpoint.startsWith('POST ')) { + labels.add((options.labels as string[])[0]); + return {}; + } + if (endpoint.startsWith('DELETE ')) { + labels.delete(options.name as string); + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + context.octokit.request = request as never; + + const olderResult = await safeUpdateLabels(context, [], [], { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: redis as never, + }); + await newerPromise; + + assert.strictEqual(newerResult?.success, true); + assert.strictEqual(olderResult.success, false); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-gemini-3-pro']); + assert.deepStrictEqual(olderResult.finalLabels?.sort(), ['AI', 'llm-claude-opus48']); +}); + +test('serializes a newer transition started after rollback ownership read and before restoration mutations', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + const redis = createTransitionRedis(); + let issueReads = 0; + let newerPromise: Promise>> | undefined; + let newerResult: Awaited> | undefined; + let newerSucceededBeforeRestorationMutation = false; + const transitionOrder: string[] = []; + const context = { octokit: { request: undefined as never }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) { + issueReads += 1; + if (issueReads === 2) throw new Error('older transition verification failed'); + const snapshot = [...labels]; + if (issueReads === 3) { + transitionOrder.push('A-rollback-ownership-read'); + newerPromise = safeUpdateLabels(context, [], [], { + targetLabel: 'llm-gemini-3-pro', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: redis as never, + }).then(result => { + newerResult = result; + transitionOrder.push('B-success'); + return result; + }); + transitionOrder.push('B-started'); + await Promise.resolve(); + } + return { data: { labels: snapshot } }; + } + if (endpoint.startsWith('POST ')) { + const label = (options.labels as string[])[0]; + if (label === 'llm-claude-opus48') { + newerSucceededBeforeRestorationMutation = newerResult?.success === true; + transitionOrder.push('A-restoration-mutation'); + } + labels.add(label); + return {}; + } + if (endpoint.startsWith('DELETE ')) { + labels.delete(options.name as string); + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + context.octokit.request = request as never; + + const olderResult = await safeUpdateLabels(context, [], [], { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: redis as never, + }); + transitionOrder.push('A-complete'); + await newerPromise; + + assert.strictEqual(newerSucceededBeforeRestorationMutation, false); + assert.strictEqual(olderResult.success, false); + assert.strictEqual(newerResult?.success, true); + assert.deepStrictEqual(olderResult.finalLabels?.sort(), ['AI', 'llm-claude-opus48']); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-gemini-3-pro']); + assert.deepStrictEqual(transitionOrder, [ + 'A-rollback-ownership-read', + 'B-started', + 'A-restoration-mutation', + 'A-complete', + 'B-success', + ]); +}); + +test('lease loss during convergence fences all later managed-label mutations', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + const mutations: string[] = []; + let leaseOwned = true; + const lease = { + identity: { owner: 'integry', repo: 'propr', pr: 42 }, + assertOwned: async () => { + if (!leaseOwned) throw new Error('lease lost during label mutation'); + }, + }; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) return { data: { labels: [...labels] } }; + mutations.push(`${endpoint}:${String((options.labels as string[] | undefined)?.[0] ?? options.name)}`); + if (endpoint.startsWith('POST ')) { + labels.add((options.labels as string[])[0]); + // The lease expires while the add is in flight and a newer owner + // establishes its selection before the stale request returns. + leaseOwned = false; + labels.clear(); + labels.add('AI'); + labels.add('llm-gemini-3-pro'); + return {}; + } + if (endpoint.startsWith('DELETE ')) labels.delete(options.name as string); + return {}; + }); + + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + [], [], + { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + redis: createTransitionRedis() as never, + lease, + }, + ); + + assert.strictEqual(result.success, false); + assert.deepStrictEqual(mutations, [ + 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels:llm-codex-gpt56-sol', + ]); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-gemini-3-pro']); +}); + +test('lease loss during restoration cannot continue by deleting a newer selection', async () => { + const labels = new Set(['AI', 'llm-claude-opus48']); + const mutations: string[] = []; + let leaseOwned = true; + const lease = { + identity: { owner: 'integry', repo: 'propr', pr: 42 }, + assertOwned: async () => { + if (!leaseOwned) throw new Error('lease lost during restoration mutation'); + }, + }; + const request = mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) return { data: { labels: [...labels] } }; + const label = String((options.labels as string[] | undefined)?.[0] ?? options.name); + mutations.push(`${endpoint}:${label}`); + if (endpoint.startsWith('POST ')) { + labels.add(label); + if (label === 'llm-claude-opus48') { + leaseOwned = false; + labels.clear(); + labels.add('AI'); + labels.add('llm-claude-opus48'); + labels.add('llm-gemini-3-pro'); + } + return {}; + } + labels.delete(label); + if (label === 'llm-claude-opus48') labels.delete('llm-codex-gpt56-sol'); + return {}; + }); + + const result = await safeUpdateLabels( + { octokit: { request }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + [], [], + { + targetLabel: 'llm-codex-gpt56-sol', + isManagedLabel: label => label.startsWith('llm-'), + maxAttempts: 1, + redis: createTransitionRedis() as never, + lease, + }, + ); + + assert.strictEqual(result.success, false); + assert.deepStrictEqual(mutations, [ + 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels:llm-codex-gpt56-sol', + 'DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}:llm-claude-opus48', + 'POST /repos/{owner}/{repo}/issues/{issue_number}/labels:llm-claude-opus48', + ]); + assert.deepStrictEqual([...labels].sort(), ['AI', 'llm-claude-opus48', 'llm-gemini-3-pro']); +}); + +test('exclusive transition leases do not serialize unrelated PRs', async () => { + const redis = createTransitionRedis(); + let releaseFirstMutation!: () => void; + let firstMutationStarted!: () => void; + const firstMutationGate = new Promise(resolve => { releaseFirstMutation = resolve; }); + const firstMutationStart = new Promise(resolve => { firstMutationStarted = resolve; }); + const firstLabels = new Set(['llm-claude-opus48']); + const secondLabels = new Set(['llm-claude-opus48']); + const createRequest = (labels: Set, blockMutation: boolean) => mock.fn(async (endpoint: string, options: Record) => { + if (endpoint.startsWith('GET ')) return { data: { labels: [...labels] } }; + if (endpoint.startsWith('POST ')) { + labels.add((options.labels as string[])[0]); + if (blockMutation) { + firstMutationStarted(); + await firstMutationGate; + } + return {}; + } + if (endpoint.startsWith('DELETE ')) { + labels.delete(options.name as string); + return {}; + } + throw new Error(`Unexpected endpoint: ${endpoint}`); + }); + const first = safeUpdateLabels( + { octokit: { request: createRequest(firstLabels, true) }, owner: 'integry', repo: 'propr', issueNumber: 42, logger }, + [], [], + { targetLabel: 'llm-codex-gpt56-sol', isManagedLabel: label => label.startsWith('llm-'), redis: redis as never }, + ); + await firstMutationStart; + const second = safeUpdateLabels( + { octokit: { request: createRequest(secondLabels, false) }, owner: 'integry', repo: 'propr', issueNumber: 43, logger }, + [], [], + { targetLabel: 'llm-gemini-3-pro', isManagedLabel: label => label.startsWith('llm-'), redis: redis as never }, + ); + const secondWhileFirstHeld = await Promise.race([ + second, + new Promise(resolve => setTimeout(() => resolve(undefined), 50)), + ]); + releaseFirstMutation(); + const [firstResult, secondResult] = await Promise.all([first, second]); + + assert.strictEqual(secondWhileFirstHeld?.success, true); + assert.strictEqual(firstResult.success, true); + assert.strictEqual(secondResult.success, true); +}); diff --git a/test/modelAliases.test.ts b/test/modelAliases.test.ts index b75dc7690..3f9c3b21f 100644 --- a/test/modelAliases.test.ts +++ b/test/modelAliases.test.ts @@ -11,6 +11,7 @@ const { getPreferredModelForAgent, MODEL_ALIASES, resolveLlmLabel, + resolveCanonicalModelSelection, ALL_MODELS, findMatchingModel, getModelShortName, @@ -180,6 +181,82 @@ test('resolveLlmLabel - 7-step model resolution', async (t) => { assert.strictEqual(result.model, 'gpt-5.6-sol', 'Should resolve to GPT-5.6 Sol'); }); + await t.test('canonicalizes GPT-5.6 Sol aliases and full labels to one configured label', async () => { + for (const token of ['gpt-5.6-sol', 'codex-gpt56-sol', 'llm-codex-gpt56-sol']) { + const result = await resolveCanonicalModelSelection(token); + assert.deepStrictEqual(result, { + agentAlias: 'codex', + model: 'gpt-5.6-sol', + githubLabel: 'llm-codex-gpt56-sol', + }); + } + }); + + await t.test('canonicalizes a raw model ID against enabled agents when a disabled match is ordered first', async () => { + const disabledCodexAgent = { + config: { + ...mockAgentConfigs[2].config, + id: 'disabled-codex-agent', + alias: 'disabled-codex', + enabled: false, + }, + }; + mockAgentConfigs.unshift(disabledCodexAgent); + try { + assert.deepStrictEqual(await resolveCanonicalModelSelection('gpt-5.6-sol'), { + agentAlias: 'codex', + model: 'gpt-5.6-sol', + githubLabel: 'llm-codex-gpt56-sol', + }); + } finally { + mockAgentConfigs.shift(); + } + }); + + await t.test('canonicalizes a configured agent alias and its full model label to that agent label', async () => { + const codexProdAgent = { + config: { + ...mockAgentConfigs[2].config, + id: 'codex-agent-prod', + alias: 'codex-prod', + }, + }; + mockAgentConfigs.push(codexProdAgent); + try { + for (const token of ['codex-prod:gpt-5.6-sol', 'llm-codex-prod-gpt56-sol']) { + assert.deepStrictEqual(await resolveCanonicalModelSelection(token), { + agentAlias: 'codex-prod', + model: 'gpt-5.6-sol', + githubLabel: 'llm-codex-prod-gpt56-sol', + }); + } + } finally { + mockAgentConfigs.pop(); + } + }); + + await t.test('uses a configured per-model custom label as the canonical label', async () => { + mockAgentConfigs[2].config.modelCustomLabels = { 'gpt-5.6-sol': 'codex-production' }; + try { + const byModel = await resolveCanonicalModelSelection('gpt-5.6-sol'); + const byCustomLabel = await resolveCanonicalModelSelection('codex-production'); + assert.strictEqual(byModel?.githubLabel, 'codex-production'); + assert.deepStrictEqual(byCustomLabel, byModel); + } finally { + delete mockAgentConfigs[2].config.modelCustomLabels; + } + }); + + await t.test('rejects unknown and disabled model selections', async () => { + assert.strictEqual(await resolveCanonicalModelSelection('not-a-real-model'), null); + mockAgentConfigs[2].config.enabled = false; + try { + assert.strictEqual(await resolveCanonicalModelSelection('gpt-5.6-sol'), null); + } finally { + mockAgentConfigs[2].config.enabled = true; + } + }); + await t.test('routes an alias-aware GPT-5.6 Sol label to the selected Codex agent', async () => { const codex2Agent = { config: { diff --git a/test/prCommentAgentUtils.test.ts b/test/prCommentAgentUtils.test.ts index 7be243c63..7f5173077 100644 --- a/test/prCommentAgentUtils.test.ts +++ b/test/prCommentAgentUtils.test.ts @@ -10,7 +10,7 @@ const privateKeyPath = join(tmpdir(), 'propr-test-private-key.pem'); writeFileSync(privateKeyPath, '-----BEGIN PRIVATE KEY-----\ntest\n-----END PRIVATE KEY-----\n'); process.env.GH_PRIVATE_KEY_PATH ||= privateKeyPath; process.env.DEFAULT_CLAUDE_MODEL ||= 'haiku'; -const { generateSummaryTitle, resolveAndExecuteAgent } = await import('../src/jobs/prCommentAgentUtils.js'); +const { generateSummaryTitle, isProviderLimitRetrySuperseded, resolveAndExecuteAgent } = await import('../src/jobs/prCommentAgentUtils.js'); const { AgentRegistry } = await import('@propr/core'); const { db } = await import('@propr/core'); @@ -43,6 +43,54 @@ function baseOptions(overrides = {}) { }; } +test('provider-limit retry routing rejects transitional and different labels but allows its matching singleton', async (t) => { + const registry = AgentRegistry.getInstance(); + const agents = [ + { + config: { + alias: 'claude', + type: 'claude', + enabled: true, + defaultModel: 'claude-opus-4-8', + supportedModels: ['claude-opus-4-8'], + }, + }, + { + config: { + alias: 'codex', + type: 'codex', + enabled: true, + defaultModel: 'gpt-5.6-sol', + supportedModels: ['gpt-5.6-sol'], + }, + }, + ]; + t.mock.method(registry, 'ensureInitialized', async () => undefined); + t.mock.method(registry, 'getAllAgents', () => agents as never); + + const retry = { + isRetryFromRateLimit: true, + agentAlias: 'claude', + modelName: 'claude-opus-4-8', + pullRequestNumber: 1897, + correlatedLogger: logger as never, + }; + + assert.strictEqual(await isProviderLimitRetrySuperseded([ + 'AI', + 'llm-claude-opus48', + 'llm-codex-gpt56-sol', + ], retry), true, 'old and target labels visible during convergence must supersede the retry'); + assert.strictEqual(await isProviderLimitRetrySuperseded([ + 'AI', + 'llm-claude-opus48', + ], retry), false, 'the retry matching the one managed label remains allowed'); + assert.strictEqual(await isProviderLimitRetrySuperseded([ + 'AI', + 'llm-codex-gpt56-sol', + ], retry), true, 'a different managed-label singleton must supersede the retry'); +}); + describe('generateSummaryTitle fallback behavior', () => { test('returns deterministic fallback for empty context without invoking the LLM', async () => { let analysisCalls = 0; diff --git a/test/prCommentRoutingPersistence.test.ts b/test/prCommentRoutingPersistence.test.ts new file mode 100644 index 000000000..3cb3e988e --- /dev/null +++ b/test/prCommentRoutingPersistence.test.ts @@ -0,0 +1,82 @@ +import { test, after } from 'node:test'; +import assert from 'node:assert'; + +import { buildProviderLimitRetryJobData } from '../src/jobs/prCommentRouting.js'; +import { + buildProviderLimitRetryJobId, + buildRuntimeProviderLimitRetryJobData, +} from '../src/jobs/prCommentJobUtils.js'; +import { closeConnection } from '../packages/core/src/db/connection.js'; +import { shutdownQueue } from '../packages/core/src/queue/taskQueue.js'; + +after(async () => { + await shutdownQueue(); + await closeConnection(); +}); + +test('provider-limit retry reconstruction preserves the full claimed comment set and explicit slash-command routing', () => { + const retry = buildProviderLimitRetryJobData({ + pullRequestNumber: 42, + repoOwner: 'integry', + repoName: 'propr', + correlationId: 'routing-test', + comments: [ + { id: 10, body: 'normal request', author: 'alice', type: 'issue' }, + { id: 20, body: 'selected request', author: 'alice', type: 'issue', commandMode: 'use', hasCodeContext: true }, + ], + llm: 'gpt-5.6-sol', + agentAlias: 'codex', + modelName: 'gpt-5.6-sol', + modelLabel: 'llm-codex-gpt56-sol', + }); + + assert.strictEqual(retry.isRetryFromRateLimit, true); + assert.strictEqual(retry.agentAlias, 'codex'); + assert.strictEqual(retry.modelName, 'gpt-5.6-sol'); + assert.strictEqual(retry.modelLabel, 'llm-codex-gpt56-sol'); + assert.strictEqual(retry.llm, 'gpt-5.6-sol'); + assert.deepStrictEqual(retry.comments?.map(comment => comment.id), [10, 20]); + assert.strictEqual(retry.comments?.[1].hasCodeContext, true); +}); + +test('ordinary provider-limit retry persists the failed attempt runtime route in its payload and identity', () => { + const retry = buildRuntimeProviderLimitRetryJobData({ + pullRequestNumber: 42, + repoOwner: 'integry', + repoName: 'propr', + branchName: 'feature/routing', + correlationId: 'runtime-routing-test', + comments: [{ id: 10, body: 'normal request', author: 'alice', type: 'issue' }], + llm: 'claude-opus-4-8', + }, { + runtimeAgentAlias: 'codex-prod', + runtimeModelName: 'gpt-5.6-sol', + }); + + assert.strictEqual(retry.isRetryFromRateLimit, true); + assert.strictEqual(retry.agentAlias, 'codex-prod'); + assert.strictEqual(retry.modelName, 'gpt-5.6-sol'); + assert.strictEqual(retry.llm, 'gpt-5.6-sol'); + assert.strictEqual( + buildProviderLimitRetryJobId(retry), + 'pr-comments-batch-integry-propr-42-codex-prod-gpt-5-6-sol-feature-routing-ratelimit-retry', + ); +}); + +test('provider-limit retry retains an available canonical model label', () => { + const retry = buildRuntimeProviderLimitRetryJobData({ + pullRequestNumber: 42, + repoOwner: 'integry', + repoName: 'propr', + correlationId: 'canonical-label-test', + llm: 'gpt-5.6-sol', + agentAlias: 'codex-prod', + modelName: 'gpt-5.6-sol', + modelLabel: 'llm-codex-gpt56-sol', + }, { + runtimeAgentAlias: 'codex-prod', + runtimeModelName: 'gpt-5.6-sol', + }); + + assert.strictEqual(retry.modelLabel, 'llm-codex-gpt56-sol'); +}); diff --git a/test/prPendingCommentsDurability.test.ts b/test/prPendingCommentsDurability.test.ts new file mode 100644 index 000000000..2b2cf6ff4 --- /dev/null +++ b/test/prPendingCommentsDurability.test.ts @@ -0,0 +1,264 @@ +import { test, mock, after } from 'node:test'; +import assert from 'node:assert'; +import { getUnprocessedCommentIdentity, type UnprocessedComment } from '@propr/core'; +import { + acknowledgePendingCommentClaim, + pickUpPendingCommentsWithClaim, + processPendingComments, + restorePendingComments, + restoreSupersededProviderLimitComments, +} from '../src/jobs/prPendingComments.js'; +import { applyPendingCommentCommandContext } from '../src/jobs/prCommentCommandContext.js'; +import { buildProviderLimitRetryJobData } from '../src/jobs/prCommentRouting.js'; +import { closeConnection } from '../packages/core/src/db/connection.js'; +import { shutdownQueue } from '../packages/core/src/queue/taskQueue.js'; + +const logger = { info: mock.fn(), warn: mock.fn(), error: mock.fn(), debug: mock.fn() }; + +after(async () => { + await shutdownQueue(); + await closeConnection(); +}); + +class MemoryRedis { + lists = new Map(); + + async lrange(key: string): Promise { + return [...(this.lists.get(key) ?? [])]; + } + + async del(key: string): Promise { + return this.lists.delete(key) ? 1 : 0; + } + + async eval(_script: string, keyCount: number, ...args: string[]): Promise { + if (keyCount === 2) { + const [pendingKey, claimKey] = args; + const existingClaim = this.lists.get(claimKey); + if (existingClaim) return [...existingClaim]; + const pending = [...(this.lists.get(pendingKey) ?? [])]; + if (pending.length > 0) { + this.lists.delete(pendingKey); + this.lists.set(claimKey, pending); + } + return pending; + } + + const [pendingKey, ...identityPayloadPairs] = args; + const existing = this.lists.get(pendingKey) ?? []; + const identity = (raw: string): string => { + const value = JSON.parse(raw) as UnprocessedComment; + return getUnprocessedCommentIdentity(value); + }; + const seen = new Set(existing.map(identity)); + const missing: string[] = []; + for (let index = 0; index < identityPayloadPairs.length; index += 3) { + if (!seen.has(identityPayloadPairs[index])) { + seen.add(identityPayloadPairs[index]); + missing.push(identityPayloadPairs[index + 2]); + } + } + this.lists.set(pendingKey, [...missing, ...existing]); + return missing.length; + } +} + +function issue(id: number, body: string, updatedAt: string): UnprocessedComment { + return { id, body, updatedAt, author: 'alice', type: 'issue' }; +} + +test('picked /use comment is persisted with the normal comment before provider retry reconstruction', () => { + const jobData = { + pullRequestNumber: 42, + repoOwner: 'integry', + repoName: 'propr', + correlationId: 'claimed-routing', + comments: [issue(10, 'normal request', 'r1')], + llm: 'claude-opus-4-8', + }; + const comments = [...jobData.comments]; + processPendingComments(comments, [JSON.stringify({ + ...issue(20, 'selected follow-up', 'r1'), + commandMode: 'use', + requestedModels: ['gpt-5.6-sol'], + llmOverride: 'gpt-5.6-sol', + agentAlias: 'codex', + modelName: 'gpt-5.6-sol', + modelLabel: 'llm-codex-gpt56-sol', + })], logger as never); + + applyPendingCommentCommandContext(jobData, comments, logger as never); + const retry = buildProviderLimitRetryJobData(jobData); + + assert.deepStrictEqual(retry.comments?.map(comment => comment.id), [10, 20]); + assert.strictEqual(retry.agentAlias, 'codex'); + assert.strictEqual(retry.modelName, 'gpt-5.6-sol'); + assert.strictEqual(retry.modelLabel, 'llm-codex-gpt56-sol'); + assert.strictEqual(retry.llm, 'gpt-5.6-sol'); +}); + +test('comment identity preserves issue/review ID collisions, revisions, order, and code context', () => { + const comments = [issue(7, 'issue request', 'issue-r1')]; + processPendingComments(comments, [ + JSON.stringify({ id: 7, body: 'review r1', updatedAt: 'review-r1', author: 'bob', type: 'review', hasCodeContext: true }), + JSON.stringify({ id: 7, body: 'review r1', updatedAt: 'review-r1', author: 'bob', type: 'review', hasCodeContext: true }), + JSON.stringify({ id: 7, body: 'review changed at same timestamp', updatedAt: 'review-r1', author: 'bob', type: 'review', hasCodeContext: true }), + JSON.stringify({ id: 7, body: 'review r2', updatedAt: 'review-r2', author: 'bob', type: 'review', hasCodeContext: true }), + ], logger as never); + + assert.deepStrictEqual(comments.map(comment => `${comment.type}:${comment.id}:${comment.updatedAt}`), [ + 'issue:7:issue-r1', + 'review:7:review-r1', + 'review:7:review-r1', + 'review:7:review-r2', + ]); + assert.deepStrictEqual(comments.filter(comment => comment.type === 'review').map(comment => comment.hasCodeContext), [true, true, true]); +}); + +test('execution receives only the newest revision per exact comment type and ID while job data retains every revision', () => { + const revisions: UnprocessedComment[] = [ + issue(7, 'issue r2', '2026-08-14T10:03:00Z'), + { id: 7, body: 'review r1', updatedAt: '2026-08-14T10:02:00Z', author: 'bob', type: 'review', hasCodeContext: true }, + issue(7, 'issue r1', '2026-08-14T10:01:00Z'), + { id: 7, body: 'review r2', updatedAt: '2026-08-14T10:04:00Z', author: 'bob', type: 'review', hasCodeContext: true }, + ]; + const jobData = { + pullRequestNumber: 42, + repoOwner: 'integry', + repoName: 'propr', + correlationId: 'latest-comment-revisions', + }; + + const commentsForExecution = applyPendingCommentCommandContext(jobData, revisions, logger as never); + + assert.deepStrictEqual(commentsForExecution.map(comment => `${comment.type}:${comment.id}:${comment.body}`), [ + 'issue:7:issue r2', + 'review:7:review r2', + ]); + assert.deepStrictEqual(jobData.comments.map(comment => `${comment.type}:${comment.id}:${comment.body}`), [ + 'issue:7:issue r2', + 'review:7:review r1', + 'issue:7:issue r1', + 'review:7:review r2', + ]); +}); + +test('out-of-order revisions of one edited /use comment keep the newest model selection', () => { + const createdAt = '2026-08-14T10:00:00Z'; + const newer = { + ...issue(20, '/use codex', '2026-08-14T10:05:00Z'), + createdAt, + commandMode: 'use' as const, + requestedModels: ['gpt-5.6-sol'], + llmOverride: 'gpt-5.6-sol', + agentAlias: 'codex', + modelName: 'gpt-5.6-sol', + modelLabel: 'llm-codex-gpt56-sol', + }; + const older = { + ...issue(20, '/use opus', '2026-08-14T10:01:00Z'), + createdAt, + commandMode: 'use' as const, + requestedModels: ['claude-opus-4-8'], + llmOverride: 'claude-opus-4-8', + agentAlias: 'claude', + modelName: 'claude-opus-4-8', + modelLabel: 'llm-claude-opus48', + }; + const jobData = { + pullRequestNumber: 42, repoOwner: 'integry', repoName: 'propr', correlationId: 'edited-use-order', + commandMode: 'default' as const, llm: 'initial-model', + }; + + applyPendingCommentCommandContext(jobData, [newer, older], logger as never); + + assert.strictEqual(jobData.llm, 'gpt-5.6-sol'); + assert.strictEqual(jobData.modelLabel, 'llm-codex-gpt56-sol'); + assert.strictEqual(jobData.commandCommentUpdatedAt, newer.updatedAt); +}); + +test('same-timestamp edit ties use durable ingestion order without collapsing either body', () => { + const timestamp = '2026-08-14T10:05:00Z'; + const first = { + ...issue(20, '/use opus', timestamp), + createdAt: '2026-08-14T10:00:00Z', + commandMode: 'use' as const, + requestedModels: ['claude-opus-4-8'], + llmOverride: 'claude-opus-4-8', + modelLabel: 'llm-claude-opus48', + }; + const liveTieWinner = { + ...issue(20, '/use codex', timestamp), + createdAt: '2026-08-14T10:00:00Z', + commandMode: 'use' as const, + requestedModels: ['gpt-5.6-sol'], + llmOverride: 'gpt-5.6-sol', + modelLabel: 'llm-codex-gpt56-sol', + }; + const jobData = { + pullRequestNumber: 42, repoOwner: 'integry', repoName: 'propr', correlationId: 'edited-use-tie', + commandMode: 'default' as const, llm: 'initial-model', + }; + + applyPendingCommentCommandContext(jobData, [first, liveTieWinner], logger as never); + + assert.strictEqual(jobData.llm, 'gpt-5.6-sol'); + assert.strictEqual(jobData.modelLabel, 'llm-codex-gpt56-sol'); + assert.strictEqual(jobData.comments?.length, 2); + assert.notStrictEqual( + getUnprocessedCommentIdentity(jobData.comments![0]), + getUnprocessedCommentIdentity(jobData.comments![1]), + ); +}); + +test('claim replay and repeated restoration are idempotent and crash recoverable', async () => { + const redis = new MemoryRedis(); + const pendingKey = 'pending-pr-comments:integry:propr:42'; + const pendingUse = issue(20, 'use request', 'r1'); + redis.lists.set(pendingKey, [JSON.stringify(pendingUse)]); + const options = { + repoOwner: 'integry', repoName: 'propr', pullRequestNumber: 42, + correlatedLogger: logger as never, redisClient: redis as never, claimId: 'job-1', + }; + + const firstAttempt = await pickUpPendingCommentsWithClaim([issue(10, 'normal', 'r1')], options); + assert.deepStrictEqual(firstAttempt.commentsToProcess.map(comment => comment.id), [10, 20]); + assert.strictEqual(redis.lists.has(pendingKey), false); + + // Simulate a crash before BullMQ data is updated/claim is acknowledged. + const redelivery = await pickUpPendingCommentsWithClaim([issue(10, 'normal', 'r1')], options); + assert.deepStrictEqual(redelivery.commentsToProcess.map(comment => comment.id), [10, 20]); + await acknowledgePendingCommentClaim(options); + + await restorePendingComments(redelivery.commentsToProcess, options); + await restorePendingComments(redelivery.commentsToProcess, options); + const restored = redis.lists.get(pendingKey)!.map(raw => JSON.parse(raw) as UnprocessedComment); + assert.deepStrictEqual(restored.map(comment => comment.id), [10, 20]); +}); + +test('a delayed provider retry activated during /use supersession restores ownership and drops stale routing', async () => { + const redis = new MemoryRedis(); + const context = { + commentsToProcess: [issue(10, 'original retry request', 'r1')], + llm: 'claude-opus-4-8', + agentAlias: 'claude', + modelName: 'claude-opus-4-8', + modelLabel: 'llm-claude-opus48', + }; + const options = { + repoOwner: 'integry', repoName: 'propr', pullRequestNumber: 42, + redisClient: redis as never, + }; + + await restoreSupersededProviderLimitComments(context, options); + await restoreSupersededProviderLimitComments(context, options); + + assert.deepStrictEqual( + redis.lists.get('pending-pr-comments:integry:propr:42')!.map(raw => (JSON.parse(raw) as UnprocessedComment).id), + [10], + ); + assert.strictEqual(context.llm, null); + assert.strictEqual(context.agentAlias, undefined); + assert.strictEqual(context.modelName, undefined); + assert.strictEqual(context.modelLabel, undefined); +}); diff --git a/test/processMergeConflictJob.test.ts b/test/processMergeConflictJob.test.ts index 009bab555..cd9f4acad 100644 --- a/test/processMergeConflictJob.test.ts +++ b/test/processMergeConflictJob.test.ts @@ -173,6 +173,7 @@ await mock.module('@propr/core', { createLogFiles: mock.fn(async () => {}), UsageLimitError: class UsageLimitError extends Error { name = 'UsageLimitError'; }, AgentRegistry: { getInstance: mock.fn(() => mockRegistry) }, + resolveCanonicalModelSelectionFromLabels: mock.fn(async () => null), resolveConfiguredModel: mock.fn(async (model: string) => model), resolveLlmLabel: mock.fn(async (label: string) => ({ agentAlias: 'claude', model: label })), recordLLMMetrics: mock.fn(async () => {}), diff --git a/test/slashCommandsBlock.test.ts b/test/slashCommandsBlock.test.ts index e1cb4d7c0..d3ad96e81 100644 --- a/test/slashCommandsBlock.test.ts +++ b/test/slashCommandsBlock.test.ts @@ -49,10 +49,12 @@ describe('buildSlashCommandsBlock', () => { assert.ok(switchLine.toLowerCase().includes('change') || switchLine.toLowerCase().includes('model')); }); - test('/use description mentions single or override', () => { + test('/use description mentions switching the PR model and running a follow-up', () => { const result = buildSlashCommandsBlock(); const useLine = result.split('\n').find(l => l.includes('`/use`')); assert.ok(useLine); - assert.ok(useLine.toLowerCase().includes('single') || useLine.toLowerCase().includes('override')); + assert.ok(useLine.toLowerCase().includes('switch')); + assert.ok(useLine.toLowerCase().includes('pr model')); + assert.ok(useLine.toLowerCase().includes('follow-up')); }); }); diff --git a/test/ultrafixJobCurrent.test.ts b/test/ultrafixJobCurrent.test.ts index 8aadde084..4996d66ab 100644 --- a/test/ultrafixJobCurrent.test.ts +++ b/test/ultrafixJobCurrent.test.ts @@ -12,6 +12,20 @@ await mock.module('@propr/core', { areAllChecksPassing: mock.fn(async () => true), getCurrentPRHead: mock.fn(async () => 'head-sha'), getPendingPrCommentsKey: (owner: string, repo: string, pr: number) => `pending-pr-comments:${owner}:${repo}:${pr}`, + getUnprocessedCommentIdentity: (comment: { type: string; id: number; updatedAt?: string; createdAt?: string }) => + `${comment.type}:${comment.id}:${comment.updatedAt ?? comment.createdAt ?? ''}`, + getUnprocessedCommentRevisionIdentity: (comment: { updatedAt?: string; createdAt?: string }) => + comment.updatedAt ?? comment.createdAt ?? '', + dedupeUnprocessedComments: (comments: Array<{ type: string; id: number; updatedAt?: string; createdAt?: string }>) => + comments.filter((comment, index) => comments.findIndex(candidate => + `${candidate.type}:${candidate.id}:${candidate.updatedAt ?? candidate.createdAt ?? ''}` + === `${comment.type}:${comment.id}:${comment.updatedAt ?? comment.createdAt ?? ''}`) === index), + restorePendingCommentsIdempotently: async (redisClient: { lpush: (key: string, ...values: string[]) => Promise; expire: (key: string, ttl: number) => Promise }, key: string, comments: unknown[]) => { + if (comments.length === 0) return 0; + await redisClient.lpush(key, ...comments.map(comment => JSON.stringify(comment)).reverse()); + await redisClient.expire(key, 3600); + return comments.length; + }, issueQueue: { add: mockIssueQueueAdd, getActive: async () => mockActiveQueueJobs,