Skip to content

fix(openrouter): bound provider diagnostic body reads - #560

Open
vincent-peng wants to merge 1 commit into
steipete:mainfrom
vincent-peng:codex/fix-openrouter-diagnostic-timeout
Open

vincent-peng wants to merge 1 commit into
steipete:mainfrom
vincent-peng:codex/fix-openrouter-diagnostic-timeout

Conversation

@vincent-peng

Copy link
Copy Markdown
Contributor

Summary

Named OpenRouter presets such as free can wait beyond the configured timeout while fetching optional provider suggestions: the old deadline ended when headers arrived, before JSON parsing.

Keep the timeout signal active through the response body. Remove the now-unused private header-only wrapper and replace its unit tests with HTTP and CLI regressions. Routing errors, their original cause, and free-preset guidance remain intact.

Behavior proof

Normal built dist/cli.js processes at the exact base and head, using a local HTTP fault fixture and a synthetic key. The transport preload rewrites request URLs only; source modules run unchanged.

The fixture returns HTTP 404 with No allowed providers are available for the selected model. for the model request. Its metadata response sends HTTP 200 headers and partial JSON immediately, then delays the remaining JSON for 1,500 ms. The fast control sends complete JSON immediately.

Save the self-contained fixture below as reproduce.mjs. With Node 24 or newer and a built checkout, run:

node reproduce.mjs "$BUILD"
node reproduce.mjs "$BUILD" fast

Use the base and head builds for the first command; the second is the fast-response control. The script creates and removes its own HOME, config, input file, transport preload and HTTP server.

Complete standalone fixture
import assert from 'node:assert/strict';
import { execFile } from 'node:child_process';
import { createServer } from 'node:http';
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join, resolve } from 'node:path';
import { promisify } from 'node:util';

if (!process.argv[2]) throw new Error('Usage: node reproduce.mjs BUILD_ROOT [fast]');
const cli = join(resolve(process.argv[2]), 'dist/cli.js');
const fast = process.argv[3] === 'fast';
const home = await mkdtemp(join(tmpdir(), 'summarize-routing-'));
const trace = { modelRequests: 0, metadataHeadersSent: false, metadataBodyCancelled: null, metadataOperationMs: null };
const server = createServer((request, response) => {
  request.resume();
  if (request.method === 'POST') {
    trace.modelRequests++;
    response.writeHead(404, { 'content-type': 'application/json' });
    response.end(JSON.stringify({ error: { message: 'No allowed providers are available for the selected model.' } }));
    return;
  }
  const started = performance.now();
  response.writeHead(200, { 'content-type': 'application/json' });
  response.flushHeaders();
  trace.metadataHeadersSent = true;
  response.once('close', () => {
    trace.metadataBodyCancelled = !response.writableFinished;
    trace.metadataOperationMs = Math.round(performance.now() - started);
  });
  const json = JSON.stringify({ data: { endpoints: [{ provider_name: fast ? 'Fast Provider' : 'Slow Provider' }] } });
  if (fast) return response.end(json);
  response.write(json.slice(0, 8));
  const timer = setTimeout(() => response.end(json.slice(8)), 1500);
  response.once('close', () => clearTimeout(timer));
});
try {
  await new Promise(resolve => server.listen(0, '127.0.0.1', resolve));
  const origin = `http://127.0.0.1:${server.address().port}`;
  await mkdir(join(home, '.summarize'));
  await writeFile(join(home, '.summarize/config.json'), JSON.stringify({ cache: { enabled: false }, models: { free: { mode: 'auto', rules: [{ candidates: ['openrouter/vendor/model:free'] }] } } }));
  const input = join(home, 'article.txt');
  await writeFile(input, 'A scientific report about lunar instruments and calibration measurements.');
  const transport = join(home, 'transport.mjs');
  await writeFile(transport, `
const nativeFetch = globalThis.fetch.bind(globalThis);
globalThis.fetch = (input, init) => {
  const url = new URL(String(input));
  if (url.hostname !== 'openrouter.ai') throw new Error('Unexpected fixture host');
  const path = url.pathname.endsWith('/chat/completions') ? '/chat/completions' : url.pathname.endsWith('/endpoints') ? '/endpoints' : null;
  if (!path) throw new Error('Unexpected fixture path');
  return nativeFetch(process.env.SUMMARIZE_PROOF_ORIGIN + path, init);
};
`);
  const pending = promisify(execFile)(process.execPath, ['--import', transport, cli, '--model', 'free', '--timeout', '500ms', '--retries', '0', '--force-summary', '--plain', '--no-cache', input], {
    cwd: home, timeout: 20000, killSignal: 'SIGKILL',
    env: { HOME: home, PATH: '/usr/bin:/bin', LANG: 'C', OPENROUTER_API_KEY: 'fixture-key', SUMMARIZE_PROOF_ORIGIN: origin },
  });
  pending.child.stdin.end();
  try {
    await pending;
    throw new Error('Expected routing failure');
  } catch (error) {
    assert.equal(error.code, 1);
    assert.match(error.stderr, /OpenRouter could not route any models/);
    console.log(JSON.stringify({ node: process.version, fast, ...trace, exit: error.code, stdout: error.stdout, stderr: error.stderr }, null, 2));
  }
} finally {
  server.closeAllConnections();
  await new Promise(resolve => server.close(resolve));
  await rm(home, { recursive: true, force: true });
}

Captured standalone-fixture output on Node 24.22.0 below, annotated with the source commits. metadataOperationMs measures the fixture metadata response, not total CLI duration. Node 26.11.1 reproduced all three outcomes. Each run made one model request and one metadata request; addresses and local paths are normalized.

Captured before/after CLI output
[
  {
    "source": "base",
    "commit": "aaccb7c9c30e9d7a4440fe3a2aa6e8c28c777507",
    "node": "v24.22.0",
    "fast": false,
    "modelRequests": 1,
    "metadataHeadersSent": true,
    "metadataBodyCancelled": false,
    "metadataOperationMs": 1504,
    "exit": 1,
    "stdout": "",
    "stderr": "OpenRouter could not route any models with this API key (no allowed providers). Tried: openrouter/vendor/model:free. Providers to allow: Slow Provider. Hint: increase --timeout (e.g. 10m) and/or use --debug/--verbose to see per-model failures. (OpenRouter: Settings \u2192 API Keys \u2192 edit key \u2192 Allowed providers.)\nTip: run \"summarize refresh-free\" to refresh the free model candidates (writes ~/.summarize/config.json).\n"
  },
  {
    "source": "fixed",
    "commit": "e9a153cc00cead592ded714708b0e349fc919b21",
    "node": "v24.22.0",
    "fast": false,
    "modelRequests": 1,
    "metadataHeadersSent": true,
    "metadataBodyCancelled": true,
    "metadataOperationMs": 504,
    "exit": 1,
    "stdout": "",
    "stderr": "OpenRouter could not route any models with this API key (no allowed providers). Tried: openrouter/vendor/model:free. Hint: increase --timeout (e.g. 10m) and/or use --debug/--verbose to see per-model failures. (OpenRouter: Settings \u2192 API Keys \u2192 edit key \u2192 Allowed providers.)\nTip: run \"summarize refresh-free\" to refresh the free model candidates (writes ~/.summarize/config.json).\n"
  },
  {
    "source": "fixed",
    "commit": "e9a153cc00cead592ded714708b0e349fc919b21",
    "node": "v24.22.0",
    "fast": true,
    "modelRequests": 1,
    "metadataHeadersSent": true,
    "metadataBodyCancelled": false,
    "metadataOperationMs": 0,
    "exit": 1,
    "stdout": "",
    "stderr": "OpenRouter could not route any models with this API key (no allowed providers). Tried: openrouter/vendor/model:free. Providers to allow: Fast Provider. Hint: increase --timeout (e.g. 10m) and/or use --debug/--verbose to see per-model failures. (OpenRouter: Settings \u2192 API Keys \u2192 edit key \u2192 Allowed providers.)\nTip: run \"summarize refresh-free\" to refresh the free model candidates (writes ~/.summarize/config.json).\n"
  }
]

Validation

  • pnpm run build
  • pnpm run check: 3,903 passed, 43 skipped; formatting, localization, lint, type checks and coverage passed
  • 13 focused tests passed on both Node 24.22.0 and Node 26.11.1
  • Seven HTTP/CLI regressions cover body/header deadlines, cancellation, retained fast hints, metadata failures, the original error cause, free-preset guidance and empty stdout; three failed on the base before the fix

No configuration, cache-format, public API, dependency or model-default changes.

@clawsweeper

clawsweeper Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@vincent-peng
vincent-peng marked this pull request as ready for review November 8, 2026 00:56
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. labels Nov 8, 2026
@clawsweeper

clawsweeper Bot commented Nov 8, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge.

What this changes

Keep OpenRouter provider-diagnostic deadlines active through response-body reads and remove the unused header-only timeout helper.

Example: A free-preset routing failure triggers a provider lookup with --timeout 500ms, but its JSON body takes 1,500ms.

  • Before: The lookup completes after about 1,504ms and delays the routing error.
  • After: The body is cancelled after about 504ms and the CLI preserves the routing guidance without slow provider hints.

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A small, complete fix with shipped-CLI before/after evidence, meaningful HTTP regressions, passing exact-head checks, and no identified defects.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): Captured built dist/cli.js runs on Node 24.22.0 exercise the production diagnostic owner through native fetch and a local HTTP server, with URL rewriting only: the head cancels the delayed metadata body near the 500ms deadline, preserves the routing error and free-preset guidance, and retains fast provider hints.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Optional provider suggestions can delay an OpenRouter routing error beyond the configured timeout.
Reason: The change enforces the existing deadline without adding settings, changing model selection, or replacing user preferences.

Merge readiness

✅ Ready for maintainer review

This PR fixes a current timeout defect with a focused change; no actionable correctness or security findings remain.

Priority: P2
Reviewed head: e9a153cc00cead592ded714708b0e349fc919b21

Before merge

None.

Findings

None.

Agent review details

How this fits together

After named-model routing fails, the summary engine fetches optional OpenRouter provider metadata and combines available hints with the original failure guidance.

flowchart TD
A[Named model request] --> B[Routing failure]
B --> C[Summary engine]
C --> D[Provider metadata lookup]
D --> E[Deadline covers headers and body]
E --> F[Best effort provider hints]
F --> G[CLI routing guidance]
Loading

Technical review

Best possible solution:

Merge the focused deadline fix while retaining best-effort provider hints and the original routing failure.

Do we have a high-confidence way to reproduce the issue?

The base source clearly ends the deadline before JSON parsing, and the contributor supplies exact-base/head built-CLI traces using a delayed real HTTP body; no reproduction was run during this read-only review.

Is this the best way to solve the issue?

The native timeout signal fixes the lifetime error at the existing diagnostic owner, removes an obsolete private wrapper, and preserves fast hints, routing guidance, and the original error cause.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against aaccb7c9c30e.

Provenance checked

  • OpenRouter diagnostic deadline and best-effort handling keeps the original intent (a06ef6f: The runner split carried provider suggestions with a capped lookup deadline and swallowed optional lookup failures; this establishes the earlier implementation, not its original introduction.)
  • Private engine timeout helper and its tests keeps the original intent (refactor: extract headless summary engine #271: The headless-engine extraction moved timeout policy into the engine while preserving fallback behavior; the removed helper has no remaining production consumers.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

  • Pinned introduced change: The patch uses AbortSignal.timeout through response.json(), preserves best-effort error handling, and deletes the private helper whose only production consumer was this lookup. (src/engine/openrouter.ts:34, e9a153cc00ce)
  • Current-main defect: The base helper clears its timer when fetch returns headers; OpenRouter JSON consumption occurs afterward, outside that deadline. (src/engine/fetch-with-timeout.ts:42, aaccb7c9c30e)
  • Supplied shipped-CLI proof: The full PR body includes a standalone real-HTTP fixture and captured built-CLI output pinned to base and head: the stalled response completes on base, is cancelled on head, and fast provider hints remain available. (e9a153cc00ce)
  • Exact-head checks: All five checks completed successfully for the reviewed head, including test (24); GitHub reports the PR mergeable and clean. (e9a153cc00ce)
  • Feature history and intent: History across the runner and engine paths identifies Peter Steinberger as the principal area contributor; the headless-engine extraction preserved diagnostic and fallback behavior. An older commit inspection encountered a missing local object, so earlier introduction is not asserted. (src/engine/openrouter.ts:52, aaccb7c9c30e)
  • Repository policy and clean checkout: Read the complete root AGENTS.md; no nested policies, maintainer-note directory, or pr-simplify skill were present for these paths. The checkout remained clean and no target code or tests were executed. (AGENTS.md:1, e9a153cc00ce)

Preflight source

Source unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified.

Review metrics

Metric Value Why it matters
Change size Production +4/-52; tests +238/-68; docs +2 Production code shrinks; added HTTP and CLI coverage exercises distinct timeout and diagnostic-preservation failures.

Labels

Label changes:

  • add P2: Optional provider diagnostics can exceed the configured timeout after an existing routing failure; the affected path has limited blast radius.
  • add proof: sufficient: Contributor real behavior proof is sufficient.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Optional provider diagnostics can exceed the configured timeout after an existing routing failure; the affected path has limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed November 7, 2026, 8:08 PM ET / November 8, 2026, 01:08 UTC.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant