Skip to content

ci: add scout stage for large Lean OCR routing - #2137

Merged
Th0rgal merged 4 commits into
mainfrom
paloma/ocr-llm-scout-routing
Jul 9, 2026
Merged

ci: add scout stage for large Lean OCR routing#2137
Th0rgal merged 4 commits into
mainfrom
paloma/ocr-llm-scout-routing

Conversation

@Th0rgal

@Th0rgal Th0rgal commented Jul 9, 2026

Copy link
Copy Markdown
Member

Summary

Adds a short-term large-Lean scout stage to the OCR router.

  • keeps small-lean, medium-lean, and config-docs on the existing OCR paths
  • builds a bounded risk dossier for large-lean-hotspots
  • optionally calls an OpenAI-compatible scout model via OCR_SCOUT_LLM_URL, OCR_SCOUT_LLM_KEY, and OCR_SCOUT_LLM_MODEL
  • falls back to deterministic packet ranking when scout config is absent or fails
  • publishes honest packet coverage, scout status, residual risk, and the current strong packet-review blocker
  • documents that OCR 1.7.5 is not yet wired to review arbitrary Lean packet windows safely

Validation

  • node --check .github/scripts/ocr-router.js
  • node --check .github/scripts/post-ocr-review.js
  • node --check .github/scripts/test-ocr-routing.js
  • node .github/scripts/test-ocr-routing.js
  • bash -n .github/scripts/ocr-git-wrapper.sh
  • python3 -m json.tool .github/ocr/rules.json
  • PyYAML parse of .github/workflows/ocr-review.yml
  • actionlint .github/workflows/ocr-review.yml
  • local smoke against PR proof: reduce whole-contract legacy-bodies premise to per-statement interface #2128 head: routed to large-lean-hotspots, result status scout_triage, 8 packets, scout not_configured, OCR not attempted

Production test plan

After checks and Codex review are clean, merge this PR and trigger /ocr review on #2128. The expected production result is large-lean-hotspots with scout status reflecting whether scout env is configured, no full OCR burn, and a posted comment that says strong review is still required for selected packets.


Note

Medium Risk
Changes CI routing and outbound LLM calls for PR diffs (secrets, bounded dossier) on proof-hotspot paths; behavior is advisory with fallbacks, but misconfiguration or scout errors could skew triage emphasis.

Overview
Adds an optional scout stage to large-lean-hotspots OCR routing: the router builds a bounded JSON risk dossier from diff hunks and, when OCR_SCOUT_LLM_* is set in Actions (with fallback to main OCR secrets), calls a cheap OpenAI-compatible model to re-rank/reselect packets and attach triage reasons and questions for a stronger reviewer. Missing config, API failures, or invalid selections fall back to deterministic packet ranking; full OpenCodeReview still does not run on this path, and comments explicitly state strong packet review is required but blocked until a safe packet-window bridge exists.

Packet building now assigns packet_id, includes diff excerpts for the dossier, and strips Lean line/block comments before sorry/admit/axiom signals so prose in comments does not false-positive. Posted findings use scout_triage status, richer packet coverage (scout status, blocker, scout questions), and sanitized inline text. The workflow wires scout env vars; README and routing tests document and cover scout success/fallback paths and comment-aware detection.

Reviewed by Cursor Bugbot for commit 4f76d51. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
verity Ready Ready Preview, Comment Jul 9, 2026 7:53pm

Request Review

@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_913ee732-231e-4072-8ba9-68d4e8d47737)

@Th0rgal

Th0rgal commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79e5941018

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/scripts/ocr-router.js Outdated
decision.reason = `${decision.reason} Scout model ranked ${selected.length}/${deterministicPackets.length} packet(s) for stronger review.`;
decision.scout.selected_packets = selected.map(p => p.packet_id);
}
decision.scout.status = 'success';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat empty scout selections as fallback

When a configured scout returns valid JSON but an empty selected_packets list, or only packet IDs that do not match our deterministic IDs, selected.length stays zero but this line still records status: success. Because selected_packets was initialized to all deterministic packet IDs and decision.packets is left unchanged, downstream metrics/comments report the deterministic packets as scout-ranked even though the scout selected 0 packets. This can make the large-Lean coverage report materially misleading for schema drift/refusal responses; mark this as a deterministic fallback or no-selection state before advertising success.

Useful? React with 👍 / 👎.

@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f1360386-035b-4834-85a1-07a2c38a772f)

@Th0rgal

Th0rgal commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@codex review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview first-pass review

Status: success · Mode: config-docs · 6 finding(s) · 5 files · 387614 tokens · 36 tool calls

✅ Posted 6 inline comment(s).

OCR pilot metrics

  • Routing: config-docs (router-v3)
  • Changed files: 5 supported / 5 total; Lean 0, trust docs 0, workflow/scripts 5, contracts 0, docs 0
  • Changed lines: 393 supported; thresholds large Lean >=3 files or >800 lines
  • OCR: status success; comments 6; files 5; tokens 387614; tool calls 36; warnings 0; duration 280s
  • Largest changed files: .github/scripts/ocr-router.js (+246/-17), .github/scripts/test-ocr-routing.js (+106/-0), .github/scripts/post-ocr-review.js (+11/-2), .github/ocr/README.md (+6/-2), .github/workflows/ocr-review.yml (+3/-0)

Pilot mode: advisory only. Codex Review remains the merge gate.

testOversizedLeanGuarded();
await testLargeLeanScoutNotConfiguredFallsBack();
await testLargeLeanScoutSelectsPackets();
await testLargeLeanScoutEmptySelectionIsFallback();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [test · high]

The tests cover two of three scout fallback paths (not_configured and fallback_no_selection), but are missing the critical fallback_deterministic path that fires when the API call itself fails. The router's applyScoutStage wraps callScoutModel in a try-catch (router lines 323-343) and falls back to fallback_deterministic status on HTTP errors (!response.ok), network exceptions (fetch throwing), and malformed JSON (JSON.parse failures). Since this is the primary safety net for CI resilience, it should have test coverage. Consider adding a test where the mocked global.fetch returns { ok: false, status: 503, text: async () => 'Service Unavailable' } and asserting decision.scout.status === 'fallback_deterministic' and that original packets are retained.

Suggested change:

  await testLargeLeanScoutNotConfiguredFallsBack();
  await testLargeLeanScoutSelectsPackets();
  await testLargeLeanScoutEmptySelectionIsFallback();
  await testLargeLeanScoutApiFailureFallsBack();

// Example test:
// async function testLargeLeanScoutApiFailureFallsBack() {
//   const files = [
//     file('Compiler/Proofs/A.lean', 40, 0),
//     file('Compiler/Proofs/B.lean', 40, 0),
//     file('Compiler/Proofs/C.lean', 40, 0),
//   ];
//   const decision = router.decideRoute(files);
//   const originalPackets = decision.packets.map(p => p.packet_id);
//   const oldFetch = global.fetch;
//   global.fetch = async () => ({
//     ok: false,
//     status: 503,
//     text: async () => 'Service Unavailable',
//   });
//   try {
//     await router.applyScoutStage(decision, { files }, { url: 'https://example.invalid/v1', key: 'test-key', model: 'cheap-scout' });
//   } finally {
//     global.fetch = oldFetch;
//   }
//   assert.strictEqual(decision.scout.status, 'fallback_deterministic');
//   assert.deepStrictEqual(decision.packets.map(p => p.packet_id), originalPackets);
// }

await router.applyScoutStage(decision, { files }, { url: 'https://example.invalid/v1', key: 'test-key', model: 'cheap-scout' });
} finally {
global.fetch = oldFetch;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [maintainability · low]

The global.fetch save/restore-via-try/finally pattern is duplicated verbatim across testLargeLeanScoutSelectsPackets and testLargeLeanScoutEmptySelectionIsFallback. If a third scout test is added (e.g., for API failure), this duplication will grow. Consider extracting a small helper like withMockFetch(mockFn, async () => { ... }) to reduce boilerplate and ensure restoration always happens.

Suggested change:

// Example helper:
// async function withMockFetch(mockFn, body) {
//   const oldFetch = global.fetch;
//   global.fetch = mockFn;
//   try { await body(); } finally { global.fetch = oldFetch; }
// }
//
// Usage in test:
//   await withMockFetch(async () => ({ ok: true, text: async () => JSON.stringify({ ... }) }), async () => {
//     await router.applyScoutStage(decision, { files }, { url: '...', key: '...', model: '...' });
//   });

Comment thread .github/scripts/ocr-router.js Outdated
}
} catch (err) {
decision.scout.status = 'fallback_deterministic';
decision.scout.error = String(err.message || err).slice(0, 300);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [security · high]

The scout.error field is published verbatim in metrics via scout: decision.scout || null at line 677. This error string includes up to 180 characters of the raw HTTP response body from the scout API (text.slice(0, 180)). If the API endpoint echoes request headers (including the Bearer token) or request details in its error response, those secrets could be exposed in the metrics artifact or CI logs. GitHub Actions does mask known secret values, but a token embedded in a longer JSON string or echoed with different formatting would NOT be masked.

Suggested change:

    decision.scout.error = 'scout API call failed; see CI logs for details';
    decision.scout._internal_error = String(err.message || err).slice(0, 300); // for logging only, not published

Comment thread .github/scripts/ocr-router.js Outdated
`Risk signals: ${packet.signals.length ? packet.signals.join(', ') : 'hotspot path/churn'}.`,
];
if (packet.scout_reason) lines.push(`Scout reason: ${packet.scout_reason}`);
if (packet.scout_question) lines.push(`Question for stronger reviewer: ${packet.scout_question}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [security · high]

scout_reason and scout_question are LLM-generated strings derived from a dossier containing untrusted PR diff content (diff_excerpt, file paths, added-line samples). These are interpolated directly into PR comment text without any sanitization. A malicious PR author could craft diff content that triggers prompt injection, causing the LLM to output misleading or reputation-damaging text that gets posted as an inline PR comment. At minimum, these values should be passed through escapeMd-style sanitization or the raw interpolation should be replaced with a fenced code block.

Suggested change:

  if (packet.scout_reason) lines.push(`Scout reason: ${escapeText(packet.scout_reason)}`);
  if (packet.scout_question) lines.push(`Question for stronger reviewer: ${escapeText(packet.scout_question)}`);
// Where escapeText escapes markdown-control characters to prevent injection

Comment thread .github/scripts/ocr-router.js Outdated
selected_packets: deterministicPackets.map(p => p.packet_id),
residual_coverage: packetResidualRisk(decision),
strong_review_status: 'blocked_packet_input',
strong_review_blocker: 'OpenCodeReview 1.7.5 supports --from/--to full diff ranges, but this workflow does not have a safe packet/window input bridge for Lean hunks yet.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [maintainability · low]

The version string 'OpenCodeReview 1.7.5' is hardcoded in two places (here and in buildMetrics at line 680). This reference will become stale as the tool is updated, leading to misleading published metrics. Consider extracting this to a constant or deriving it from a configuration value.

Suggested change:

    strong_review_blocker: STRONG_REVIEW_BLOCKER_MESSAGE,
// Where STRONG_REVIEW_BLOCKER_MESSAGE is a module-level constant

function isCodeLine(line) {
const trimmed = String(line || '').trim();
return trimmed !== '' && !trimmed.startsWith('--');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [maintainability · low]

isCodeLine only filters empty lines and Lean line comments (--). It does not handle Lean 4 block comments (/- ... -/), meaning lines inside block comments are still scanned by signal patterns. A sorry or admit reference inside a block comment would produce a false positive risk signal, inflating packet scores and potentially misdirecting the scout model and reviewer attention.

Suggested change:

function isCodeLine(line, inBlockComment) {
  const trimmed = String(line || '').trim();
  if (inBlockComment) return false;
  return trimmed !== '' && !trimmed.startsWith('--');
// Note: full block comment tracking requires stateful parsing across hunk lines

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e2c173521

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/scripts/ocr-router.js Outdated
strong_review_blocker: 'OpenCodeReview 1.7.5 supports --from/--to full diff ranges, but this workflow does not have a safe packet/window input bridge for Lean hunks yet.',
};

if (!decision.scout.enabled || deterministicPackets.length === 0) return decision;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Report no-packet scout runs as skipped

When OCR_SCOUT_LLM_* is configured but the large-Lean route produces zero deterministic packets (for example, budget exceeded or no safe packets), this early return leaves status: 'not_configured' even though enabled is true. The posted packet coverage then says the scout is configured but not_configured, which corrupts the status/metrics for exactly the guarded large-diff cases; set a distinct skipped/no-packets status before returning.

Useful? React with 👍 / 👎.

@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_8cbad176-cb40-446e-a90f-73489e61239a)

@Th0rgal

Th0rgal commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb6076e95f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/scripts/ocr-router.js Outdated
Comment on lines +557 to +559
if (trimmed.startsWith('/-')) {
if (!trimmed.includes('-/')) inBlockComment = true;
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not drop code after inline Lean block comments

When an added Lean line starts with a closed block comment followed by real code, e.g. /- rationale -/ axiom bad : False, this branch treats the whole line as comment-only and continues. Lean comments can appear where whitespace is allowed, so dangerous code after -/ is valid but will be removed from addedCode, causing the router to miss high-risk signals such as introduced axiom, unsafe, sorry, or public declaration changes and under-rank the packet sent to scout/deterministic review.

Useful? React with 👍 / 👎.

@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_cc84651e-640f-4ffc-a02b-7e552945f48c)

@Th0rgal

Th0rgal commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f76d5170f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 525 to +528
const added = hunk.lines.filter(l => l.type === 'add').map(l => l.text);
const deleted = hunk.lines.filter(l => l.type === 'del').map(l => l.text);
const allChanged = [...added, ...deleted];
const addedCode = codeLines(added);
const deletedCode = codeLines(deleted);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve Lean block-comment state from hunk context

When a hunk adds text inside an existing Lean block comment whose /- and -/ delimiters are unchanged context, this strips comments from only the added/deleted lines, so inBlockComment starts false and comment prose is scanned as code. For example, adding a line mentioning sorry or axiom inside such a comment still raises high-risk signals and can dominate scout packet selection/post bogus findings; the scanner needs to account for context before changed lines or otherwise initialize block-comment state for the hunk.

Useful? React with 👍 / 👎.

@Th0rgal
Th0rgal merged commit 240ccc2 into main Jul 9, 2026
19 checks passed

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview first-pass review

Status: success · Mode: config-docs · 7 finding(s) · 5 files · 429081 tokens · 33 tool calls

✅ Posted 7 inline comment(s).

OCR pilot metrics

  • Routing: config-docs (router-v3)
  • Changed files: 5 supported / 5 total; Lean 0, trust docs 0, workflow/scripts 5, contracts 0, docs 0
  • Changed lines: 527 supported; thresholds large Lean >=3 files or >800 lines
  • OCR: status success; comments 7; files 5; tokens 429081; tool calls 33; warnings 0; duration 548s
  • Largest changed files: .github/scripts/ocr-router.js (+315/-18), .github/scripts/test-ocr-routing.js (+170/-0), .github/scripts/post-ocr-review.js (+11/-2), .github/ocr/README.md (+6/-2), .github/workflows/ocr-review.yml (+3/-0)

Pilot mode: advisory only. Codex Review remains the merge gate.

const signals = Array.isArray(packet.signals) && packet.signals.length ? packet.signals.join(', ') : 'hotspot path/churn';
body += ` - ${escapeMd(packet.path)}:${packet.start_line ?? '?'} score ${packet.score ?? '?'} — ${escapeMd(signals)}\n`;
const question = packet.scout_question ? `; ask: ${packet.scout_question}` : '';
body += ` - ${escapeMd(packet.path)}:${packet.start_line ?? '?'} score ${packet.score ?? '?'} — ${escapeMd(signals + question)}\n`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [security · high]

packet.scout_question is untrusted output from an external LLM scout model, but here it is rendered through escapeMd() which only escapes < and > — it does NOT neutralize GitHub @mentions, markdown link syntax, or other special characters. A malicious or misbehaving scout response could inject @user mention spam or alter the review body with injected markdown links/formatting. In ocr-router.js, this same field is properly handled with sanitizeCommentText() (which neutralizes @, escapes markdown specials, etc.) for inline comments — the same level of sanitization should be applied here before embedding it in the review body.

Suggested change:

      const question = packet.scout_question ? `; ask: ${sanitizeCommentText(packet.scout_question)}` : '';
      body += `  - ${escapeMd(packet.path)}:${packet.start_line ?? '?'} score ${packet.score ?? '?'} — ${escapeMd(signals)}${escapeMd(question)}\n`;

ok: false,
status: 503,
text: async () => 'Service Unavailable with echoed request details',
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [test · medium]

Missing test coverage for network-level fetch exceptions and the timeout/abort path. testLargeLeanScoutApiFailureFallsBack only tests HTTP-level failure (ok: false, status 503), but the implementation's classifyScoutError also handles 'timeout' (AbortController abort) and 'request_failed' (fetch throwing entirely, e.g., DNS resolution failure, connection refused). These are realistic CI failure modes that would silently break if error handling is refactored. Consider adding a test where the mock fetch throws synchronously/asynchronously, and one where it respects the AbortController signal for the 45s timeout.

Suggested change:

// Add after testLargeLeanScoutApiFailureFallsBack:
async function testLargeLeanScoutNetworkErrorFallsBack() {
  const files = [
    file('Compiler/Proofs/A.lean', 40, 0),
    file('Compiler/Proofs/B.lean', 40, 0),
    file('Compiler/Proofs/C.lean', 40, 0),
  ];
  const decision = router.decideRoute(files);
  const oldFetch = global.fetch;
  global.fetch = async () => { throw new TypeError('fetch failed'); };
  try {
    await router.applyScoutStage(decision, { files }, { url: 'https://example.invalid/v1', key: 'test-key', model: 'cheap-scout' });
  } finally {
    global.fetch = oldFetch;
  }
  assert.strictEqual(decision.scout.status, 'fallback_deterministic');
  assert.strictEqual(decision.scout.error_type, 'request_failed');
}

global.fetch = async () => ({
ok: false,
status: 503,
text: async () => 'Service Unavailable with echoed request details',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [documentation · low]

The mock response text 'Service Unavailable with echoed request details' is misleading. A reader may infer the production scout error path echoes request details (which could include the API key in the Authorization header). In reality, the implementation discards the response body on HTTP errors and only throws scout model HTTP ${status}. The test does not assert on this text, but it should be simplified to avoid confusion.

Suggested change:

    text: async () => 'Service Unavailable',

headers: {
'content-type': 'application/json',
authorization: `Bearer ${config.key}`,
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [security · high]

The API key (config.key) is sent as a Bearer token via fetch() but there is no validation that config.url (derived from OCR_SCOUT_LLM_URL) uses HTTPS. If an operator configures an http:// URL (even in an internal proxy setup), the bearer token will be transmitted in cleartext. Add an explicit HTTPS check before making the request, or at minimum log a warning when a non-HTTPS URL is used.

Suggested change:

    const scoutUrl = openAiChatUrl(config.url);
    if (!scoutUrl.startsWith('https://')) {
      throw new Error(`scout model URL must use HTTPS; got ${redactUrl(scoutUrl)}`);
    }
    const response = await fetch(scoutUrl, {
      method: 'POST',
      signal: controller.signal,
      headers: {
        'content-type': 'application/json',
        authorization: `Bearer ${config.key}`,
      },

rest = rest.slice(blockStart + 2);
const blockEnd = rest.indexOf('-/');
if (blockEnd === -1) return { code, inBlockComment: true };
rest = rest.slice(blockEnd + 2);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [bug · medium]

Lean supports nested block comments (e.g., /- outer /- inner -/ still comment -/). This parser treats the first -/ as the block-comment terminator regardless of nesting depth, so content after a premature close (the outer -/) is incorrectly treated as code. This can cause false-positive signal detection for sorry/admit/axiom/unsafe keywords that appear in the outer portion of a nested comment. Consider tracking a nesting depth counter.

Suggested change:

    code += rest.slice(0, blockStart);
    rest = rest.slice(blockStart + 2);
    // Lean supports nested block comments: track depth
    let depth = 1;
    while (depth > 0 && rest) {
      const nextOpen = rest.indexOf('/-');
      const nextClose = rest.indexOf('-/');
      if (nextClose === -1) return { code, inBlockComment: true };
      if (nextOpen !== -1 && nextOpen < nextClose) {
        depth++;
        rest = rest.slice(nextOpen + 2);
      } else {
        depth--;
        rest = rest.slice(nextClose + 2);
      }
    }

clone.candidate_packets.pop();
}
return clone;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [bug · medium]

truncateJson only reduces candidate_packets entries and pops packets from the array, but does not truncate supported_files (up to 30 entries) or top_changed_files. For diffs with many long file paths, these arrays alone could cause the serialized dossier to still exceed scoutDossierMaxChars (18000). Consider also slicing supported_files and top_changed_files during truncation to guarantee the size limit.

Suggested change:

function truncateJson(value, maxChars) {
  const json = JSON.stringify(value);
  if (json.length <= maxChars) return value;
  const clone = JSON.parse(json);
  clone.candidate_packets = (clone.candidate_packets || []).map(packet => ({
    ...packet,
    diff_excerpt: String(packet.diff_excerpt || '').slice(0, 900),
  }));
  while (JSON.stringify(clone).length > maxChars && clone.candidate_packets.length > 1) {
    clone.candidate_packets.pop();
  }
  if (JSON.stringify(clone).length > maxChars && Array.isArray(clone.supported_files)) {
    clone.supported_files = clone.supported_files.slice(0, 10);
  }
  if (JSON.stringify(clone).length > maxChars && Array.isArray(clone.top_changed_files)) {
    clone.top_changed_files = clone.top_changed_files.slice(0, 10);
  }
  return clone;
}

.replace(/[\u0000-\u001f\u007f]/g, ' ')
.replace(/@/g, '@\u200b')
.replace(/[<>]/g, ch => ({ '<': '&lt;', '>': '&gt;' }[ch]))
.replace(/([\\`*_{}\[\]()#+.!|-])/g, '\\$1')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCodeReview [maintainability · low]

This function escapes nearly every markdown special character including ., -, !, |, and +. Escaping these is unnecessary for security (only backticks, asterisks, and brackets pose markdown injection risks). The over-escaping makes review comments nearly unreadable — for example, a line number becomes L\.42 and a signal name becomes introduced sorry/admit rendered as introduced sorry\/admit. Consider limiting escaping to backticks, asterisks, underscores, and square brackets, or use a dedicated markdown-escaping library.

Suggested change:

    .replace(/([`*_\[\]])/g, '\\$1')

@Th0rgal
Th0rgal deleted the paloma/ocr-llm-scout-routing branch July 9, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant