diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 89e16ed..460ce26 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -53,3 +53,4 @@ jobs: github_token: ${{ steps.app-token.outputs.token }} openrouter_api_key: ${{ secrets.OPENROUTER_KEY }} model: ${{ vars.OPENROUTER_MODEL || 'anthropic/claude-sonnet-4-6' }} + request_timeout_seconds: ${{ vars.UMM_REQUEST_TIMEOUT_SECONDS }} diff --git a/README.md b/README.md index bb93ef6..6e536c4 100644 --- a/README.md +++ b/README.md @@ -74,25 +74,26 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Inputs -| Input | Default | Description | -| ----------------------- | ----------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | -| `openrouter_api_key` | _(required)_ | OpenRouter API key | -| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | -| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | -| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | -| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | -| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt | -| `phases` | `combined` | Review phases to run. V1 supports: `combined` | -| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | -| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | -| `priority_docs` | `README.md` | Comma-separated repo-relative paths always included in review context, independent of `trace_related_files`. Shares the doc token budget with mention-matched docs when both are active. Empty = disabled | -| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | -| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | -| `max_related_files` | `8` | Maximum import-traced related files to include in review context | -| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | -| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | -| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | +| Input | Default | Description | +| ------------------------- | ----------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | +| `openrouter_api_key` | _(required)_ | OpenRouter API key | +| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | +| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | +| `request_timeout_seconds` | `600` | Per-attempt timeout for a single model request, in seconds. A timed-out attempt aborts the HTTP call and advances the retry/fallback ladder instead of hanging the job | +| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | +| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | +| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt | +| `phases` | `combined` | Review phases to run. V1 supports: `combined` | +| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | +| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | +| `priority_docs` | `README.md` | Comma-separated repo-relative paths always included in review context, independent of `trace_related_files`. Shares the doc token budget with mention-matched docs when both are active. Empty = disabled | +| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | +| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | +| `max_related_files` | `8` | Maximum import-traced related files to include in review context | +| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | +| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | +| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | ## Outputs diff --git a/action.yml b/action.yml index 2d9e5c2..62ea023 100644 --- a/action.yml +++ b/action.yml @@ -20,6 +20,10 @@ inputs: description: Model to retry with if the primary model fails the structured-output ladder required: false default: "" + request_timeout_seconds: + description: Per-attempt timeout for a single model request, in seconds. A timed-out attempt aborts the HTTP call and advances the retry/fallback ladder instead of hanging the job. Empty = the 600 default, so workflows can wire an unset repo variable directly + required: false + default: "600" max_findings: description: Cap on posted findings, highest severity first. Empty = uncapped (all validated findings post) required: false diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 8ab6629..b0a8677 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -6,6 +6,7 @@ const makeRawInputs = (overrides: Partial = {}): RawInputs => ({ openrouterApiKey: "sk-or-testkey", model: "anthropic/claude-sonnet-4-6", fallbackModel: "", + requestTimeoutSeconds: "600", maxFindings: "", severityThreshold: "low", conventionsFile: "AGENTS.md", @@ -31,6 +32,7 @@ describe("parseConfig", () => { openrouterApiKey: "sk-or-testkey", model: "anthropic/claude-sonnet-4-6", fallbackModel: "", + requestTimeoutSeconds: 600, maxFindings: undefined, severityThreshold: "low", conventionsFile: "AGENTS.md", @@ -73,6 +75,36 @@ describe("parseConfig", () => { ) }) + it("falls back to 600 for an empty request_timeout_seconds", () => { + // Workflows wiring a bare unset repo variable pass "" — that must mean + // "use the default", not a validation failure + const config = parseConfig(makeRawInputs({ requestTimeoutSeconds: "" })) + + expect(config.requestTimeoutSeconds).toBe(600) + }) + + it("rejects a zero request_timeout_seconds", () => { + expect(() => + parseConfig(makeRawInputs({ requestTimeoutSeconds: "0" })), + ).toThrow('requestTimeoutSeconds: "0" is not a positive integer') + }) + + it("accepts a request_timeout_seconds at the timer-cap ceiling", () => { + const config = parseConfig( + makeRawInputs({ requestTimeoutSeconds: "2147483" }), + ) + + expect(config.requestTimeoutSeconds).toBe(2147483) + }) + + it("rejects a request_timeout_seconds whose milliseconds exceed the timer cap", () => { + expect(() => + parseConfig(makeRawInputs({ requestTimeoutSeconds: "2147484" })), + ).toThrow( + 'requestTimeoutSeconds: "2147484" exceeds the 2147483-second cap (2^31−1 ms timer limit)', + ) + }) + it("rejects an empty context_budget_tokens", () => { expect(() => parseConfig(makeRawInputs({ contextBudgetTokens: "" })), diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index ca99e28..0c77720 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -194,6 +194,7 @@ const baseConfig: ActionConfig = { openrouterApiKey: "sk-test", model: "test/model", fallbackModel: "", + requestTimeoutSeconds: 600, maxFindings: undefined, severityThreshold: "low", conventionsFile: "AGENTS.md", diff --git a/src/config.ts b/src/config.ts index e5e8be3..624a288 100644 --- a/src/config.ts +++ b/src/config.ts @@ -21,11 +21,35 @@ const optionalPositiveInteger = z const requiredPositiveInteger = z.string().transform(parsePositiveInteger) +/** Ceiling that keeps seconds × 1000 within the 2^31−1 ms timer cap — + * beyond it, timer implementations clamp the delay to ~1 ms and every + * request would time out instantly instead of being bounded. */ +const maxTimeoutSeconds = 2_147_483 + +/** Mirrors the action.yml default — keep the two in sync. */ +const defaultRequestTimeoutSeconds = 600 + +const timerSafeSeconds = z.string().transform((value, ctx) => { + // Empty string means "not provided": workflows wiring a bare unset repo + // variable pass "", which would otherwise override the action.yml default. + if (!value) return defaultRequestTimeoutSeconds + const parsed = parsePositiveInteger(value, ctx) + if (parsed > maxTimeoutSeconds) { + ctx.addIssue({ + code: "custom", + message: `"${value}" exceeds the ${maxTimeoutSeconds}-second cap (2^31−1 ms timer limit)`, + }) + return z.NEVER + } + return parsed +}) + const configSchema = z.object({ githubToken: z.string().min(1, "github_token is required"), openrouterApiKey: z.string().min(1, "openrouter_api_key is required"), model: z.string().min(1, "model must not be empty"), fallbackModel: z.string(), + requestTimeoutSeconds: timerSafeSeconds, maxFindings: optionalPositiveInteger, // Shape-only, like phases: the value is validated by its domain owner // (review/finding.ts resolveSeverityThreshold) at startup diff --git a/src/main.ts b/src/main.ts index 1bec8bf..1dfcaa5 100644 --- a/src/main.ts +++ b/src/main.ts @@ -31,6 +31,7 @@ const collectRawInputs = (): RawInputs => ({ openrouterApiKey: core.getInput("openrouter_api_key", { required: true }), model: core.getInput("model"), fallbackModel: core.getInput("fallback_model"), + requestTimeoutSeconds: core.getInput("request_timeout_seconds"), maxFindings: core.getInput("max_findings"), severityThreshold: core.getInput("severity_threshold"), conventionsFile: core.getInput("conventions_file"), @@ -78,7 +79,10 @@ try { generateFindings: createPromptedGenerateFindings( { openrouterClient: createOpenRouterClient( - { sdk: new OpenRouter({ apiKey: config.openrouterApiKey }) }, + { + sdk: new OpenRouter({ apiKey: config.openrouterApiKey }), + requestTimeoutMs: config.requestTimeoutSeconds * 1000, + }, logger, ), model: config.model, diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts index de82dec..a12ce83 100644 --- a/src/openrouter/__tests__/client.test.ts +++ b/src/openrouter/__tests__/client.test.ts @@ -44,8 +44,14 @@ const makeSdkStub = ({ sendResponses: StubResponse[] generationResponses?: StubResponse[] }) => { - const sendCalls: { chatRequest: ChatRequestSubset }[] = [] - const generationCalls: { id: string }[] = [] + const sendCalls: { + chatRequest: ChatRequestSubset + options: { timeoutMs?: number } | undefined + }[] = [] + const generationCalls: { + id: string + options: { timeoutMs?: number } | undefined + }[] = [] const takeNext = ( queue: StubResponse[], @@ -62,14 +68,14 @@ const makeSdkStub = ({ const sdk: OpenRouterLike = { chat: { - send: async (request) => { - sendCalls.push(request) + send: async (request, options) => { + sendCalls.push({ ...request, options }) return takeNext(sendResponses, sendCalls.length, "chat.send") }, }, generations: { - getGeneration: async (request) => { - generationCalls.push(request) + getGeneration: async (request, options) => { + generationCalls.push({ ...request, options }) return takeNext( generationResponses, generationCalls.length, @@ -84,7 +90,10 @@ const makeSdkStub = ({ const makeClient = (stub: { sdk: OpenRouterLike }) => { const logger = createTestLogger() - const client = createOpenRouterClient({ sdk: stub.sdk }, logger) + const client = createOpenRouterClient( + { sdk: stub.sdk, requestTimeoutMs: 45_000 }, + logger, + ) return { client, logger } } @@ -99,7 +108,7 @@ const requestParams = { } describe("requestReview", () => { - it("sends the strict json_schema response format with the review schema", async () => { + it("sends the strict json_schema response format with the review schema and per-attempt timeout", async () => { const stub = makeSdkStub({ sendResponses: [{ value: acceptedChatResult }] }) const { client } = makeClient(stub) @@ -124,6 +133,7 @@ describe("requestReview", () => { }, stream: false, }, + options: { timeoutMs: 45_000 }, }, ]) }) @@ -400,7 +410,9 @@ describe("requestReview", () => { const result = await client.requestReview(requestParams) - expect(stub.generationCalls).toEqual([{ id: "gen-no-cost" }]) + expect(stub.generationCalls).toEqual([ + { id: "gen-no-cost", options: { timeoutMs: 45_000 } }, + ]) expect(result.attempts[0]?.costUsd).toBe(0.0399) }) diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts index 07e9f42..ce2d7d0 100644 --- a/src/openrouter/client.ts +++ b/src/openrouter/client.ts @@ -55,10 +55,16 @@ export type ChatRequestSubset = { */ export type OpenRouterLike = { chat: { - send(request: { chatRequest: ChatRequestSubset }): Promise + send( + request: { chatRequest: ChatRequestSubset }, + options?: { timeoutMs?: number }, + ): Promise } generations?: { - getGeneration(request: { id: string }): Promise + getGeneration( + request: { id: string }, + options?: { timeoutMs?: number }, + ): Promise } } @@ -186,7 +192,16 @@ const summarizeAttempts = (attempts: ModelAttempt[]): string => attempts.map(describeAttempt).join("; ") export const createOpenRouterClient = ( - { sdk }: { sdk: OpenRouterLike }, + { + sdk, + requestTimeoutMs, + }: { + sdk: OpenRouterLike + /** Per-attempt cap — a timed-out request aborts as a status-less + * (retryable) api_error and flows into the retry/fallback ladder, + * instead of hanging the job until the runner's 6-hour kill. */ + requestTimeoutMs: number + }, logger: Logger, ): OpenRouterClient => { const attemptOnce = async ({ @@ -196,7 +211,9 @@ export const createOpenRouterClient = ( chatRequest: ChatRequestSubset model: string }): Promise => { - const sendResult = await toResult(sdk.chat.send({ chatRequest })) + const sendResult = await toResult( + sdk.chat.send({ chatRequest }, { timeoutMs: requestTimeoutMs }), + ) if (!sendResult.ok) { const statusCode = errorStatusCode(sendResult.error) const abort = statusCode !== undefined && ABORT_STATUSES.has(statusCode) @@ -295,7 +312,10 @@ export const createOpenRouterClient = ( ): Promise => { if (sdk.generations === undefined) return null const lookup = await toResult( - sdk.generations.getGeneration({ id: generationId }), + sdk.generations.getGeneration( + { id: generationId }, + { timeoutMs: requestTimeoutMs }, + ), ) if (!lookup.ok) { logger.warn("generation cost lookup failed", {