feat: Add support for Cursor Pro - #119
Conversation
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
| ], | ||
| "frontier": [ |
There was a problem hiding this comment.
There was a problem hiding this comment.
Intentional. frontier is the anchor class src/routing/intent-score.js reads (sims.frontier, FRONTIER_MIN_SIM, the REASONING band) alongside trivial/substantive/heavyweight. No change.
| // wrapper text); the force/risk probes below get envelope-stripped text | ||
| // so Cursor's <user_info>/<rules> blocks can't fire triggers on "Hi". | ||
| const isSuggestionMode = _lastUserText.includes('[SUGGESTION MODE:'); | ||
| const _lastUserAskClean = require("../routing/harness-envelope").stripHarnessEnvelope(_lastUserText); |
There was a problem hiding this comment.
Fixed in 4a56997: stripHarnessEnvelope is now a top-level require in router.js.
| // Already-degraded providers get no second chance (this attempt WAS the | ||
| // post-cooldown probe); healthy ones earn one retry before the flip. | ||
| if (embeddingProviderAvailable !== false) { | ||
| await new Promise((r) => setTimeout(r, TRANSIENT_RETRY_DELAY_MS)); |
There was a problem hiding this comment.
Leaving as-is. The retry delay itself is a setTimeout; only the degraded-provider cooldown gate reads Date.now(), and a clock step there just shifts one re-probe slightly earlier or later, with no correctness impact.
| const LYNKR_BADGE_PREFIX_RE = /^\*\[Lynkr\][^*\n]*\*\s*/; | ||
| // Mid-message badge LINES (tool narration, 2026-09-27) — strip anywhere in | ||
| // assistant content, swallowing surrounding blank lines so paragraphs reflow. | ||
| const LYNKR_BADGE_LINE_RE = /\n{0,2}\*\[Lynkr\][^*\n]*\*[ \t]*(?=\n|$)/g; |
There was a problem hiding this comment.
There was a problem hiding this comment.
No change. \n{0,2} is bounded and there's no nested or overlapping quantifier ([^*\n]* can't consume the * or newline that follows), so matching is linear with no catastrophic backtracking.
| probes.set('cursor', async () => { | ||
| const { execFileSync } = require('node:child_process'); | ||
| const binary = config.cursor?.binaryPath?.trim() || process.env.CURSOR_BINARY_PATH?.trim() || 'cursor-agent'; | ||
| execFileSync(binary, ['--version'], { timeout: PROBE_TIMEOUT_MS, stdio: 'ignore' }); |
There was a problem hiding this comment.
There was a problem hiding this comment.
The probe uses execFileSync (no shell) and the path comes from operator config/env, so there's no injection here. The review did point at a real sibling issue: cursor-utils.isAvailable ran a shell-interpolated `which ${binary}`. Fixed in 4a56997 to use execFileSync("which", [binary]).
| const UNCLOSED_RES = ENVELOPE_TAGS.map( | ||
| (t) => new RegExp(`(?:^|\\n)<${t}(?:\\s[^>]*)?>[\\s\\S]*$`, 'i') | ||
| ); |
There was a problem hiding this comment.
The UNCLOSED_RES regex lacks the 'g' flag. Without it, replace() only strips the first unclosed tag match per string. If multiple unclosed tags appear (e.g., on different lines after upstream truncation), subsequent tags remain unstripped, causing false positives in risk/force scoring.
Fix: Add the 'g' flag to the UNCLOSED_RES regex construction so all unclosed tags get replaced in one pass.
Suggestion:
| const UNCLOSED_RES = ENVELOPE_TAGS.map( | |
| (t) => new RegExp(`(?:^|\\n)<${t}(?:\\s[^>]*)?>[\\s\\S]*$`, 'i') | |
| ); | |
| const UNCLOSED_RES = ENVELOPE_TAGS.map( | |
| (t) => new RegExp(`(?:^|\\n)<${t}(?:\\s[^>]*)?>[\\s\\S]*$`, 'gi') | |
| ); |
There was a problem hiding this comment.
Not a bug. Each unclosed pattern ends in [\s\S]*$, so the first match swallows to the end of the string, including any later unclosed tags. There is nothing left for a second match, so g would be a no-op. Verified: "ask\n<rules>\na\n<rules>\nb\n<user_info>\nc" → "ask".
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | ||
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | ||
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | ||
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | ||
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') |
There was a problem hiding this comment.
Risk analyzer duplicates harness-envelope regex logic (lines 164-168). This duplicates maintenance effort and risks inconsistency between risk scoring and other scorers that use stripHarnessEnvelope.
Fix: Replace manual regex chains with require('./harness-envelope').stripHarnessEnvelope(text) in stripSystemReminders.
Suggestion:
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | |
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | |
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | |
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | |
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') | |
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | |
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | |
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | |
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | |
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') | |
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | |
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | |
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | |
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | |
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') | |
| // ... then use stripHarnessEnvelope at the top of the function instead |
There was a problem hiding this comment.
Fixed in 4a56997: stripSystemReminders now calls the shared stripHarnessEnvelope() first and the duplicated Cursor regexes are gone. Added always_applied_workspace_rules to ENVELOPE_TAGS so coverage is unchanged.
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | ||
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | ||
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | ||
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | ||
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') |
There was a problem hiding this comment.
Risk analyzer duplicates harness-envelope regex logic (lines 164-168). This creates maintenance overhead and risks inconsistency when new harness tags are added.
Fix: Replace the manual regex chain with stripHarnessEnvelope(text) from harness-envelope.js for unified stripping.
Suggestion:
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | |
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | |
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | |
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | |
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') | |
| // Use unified harness-envelope stripping instead of duplicating regex logic | |
| .replace(/<user_info>[\s\S]*?<\/user_info>/g, ' ') | |
| .replace(/<agent_transcripts>[\s\S]*?<\/agent_transcripts>/g, ' ') | |
| .replace(/<always_applied_workspace_rules?>[\s\S]*?<\/always_applied_workspace_rules?>/g, ' ') | |
| .replace(/<always_applied_workspace_rule\b[^>]*>[\s\S]*?<\/always_applied_workspace_rule>/g, ' ') | |
| .replace(/<rules>[\s\S]*?<\/rules>/g, ' ') |
| // one-shot paths. NOTE: with shell allowed, the sandbox cwd is a default | ||
| // directory, not a security boundary. Flip to false to restore the | ||
| // deny-mutations policy (MCPs-only approval, no --force). | ||
| const CURSOR_AUTO_APPROVE = true; |
There was a problem hiding this comment.
CURSOR_AUTO_APPROVE is hardcoded to true, automatically approving ALL agent permission requests including shell execution. As noted in the code comment, this bypasses security boundaries when sandbox cwd is not restricted. Consider making this configurable via env var for production deployments, or document the risk clearly in a config file.
Suggestion:
| const CURSOR_AUTO_APPROVE = true; | |
| // Configure via CURSOR_AUTO_APPROVE env var for production safety | |
| const CURSOR_AUTO_APPROVE = process.env.CURSOR_AUTO_APPROVE === 'true'; |
There was a problem hiding this comment.
Made configurable in 4a56997: CURSOR_AUTO_APPROVE env var, default true (the existing operator decision), and CURSOR_AUTO_APPROVE=false restores the deny-mutations policy. Documented with the security caveat in .env.example.
| try { | ||
| const binary = process.env.CURSOR_BINARY_PATH?.trim() || DEFAULT_BINARY; | ||
| run(`which ${binary}`, { stdio: "ignore" }); | ||
| if (!whichFn) setImmediate(() => warmupCursorAgent().catch(() => {})); |
There was a problem hiding this comment.
warmupCursorAgent catches internally, so that .catch is only a guard. The internal log was at debug, though; raised to warn in 4a56997 so a failed warmup is visible.
| function convertCursorResponseToAnthropic(text, model, usage = null, thinking = "") { | ||
| const estimatedOutputTokens = Math.ceil(String(text || "").length / 4); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Leaving as-is. The chars/4 value is only a fallback when the CLI reports no usage (ACP carries none); real usage from the CLI takes precedence. It's already named estimatedOutputTokens.
| } | ||
|
|
||
| // --- resume cache: Lynkr session key → CLI session_id ------------------------ | ||
| const _sessionCache = new Map(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
No change. node --test runs each test file in its own process, so this module state doesn't leak between files, and the in-file tests inject execFn and bypass the ACP/session caches.
- risk-analyzer: reuse stripHarnessEnvelope instead of duplicating the Cursor tag regexes; add always_applied_workspace_rules to ENVELOPE_TAGS - router: hoist harness-envelope require off the request path - cursor-utils: CURSOR_AUTO_APPROVE is now env-configurable (default true); isAvailable uses execFileSync instead of a shell-interpolated which; warmup failures log at warn; replace raw NUL in the ACP client key with an escape so the file is no longer treated as binary Co-authored-by: Cursor <cursoragent@cursor.com>
| })(), | ||
| // This branch IS the flat-fee subscription: the gate's dollar | ||
| // break-even leg has no premium to amortize here. | ||
| flatRate: true, |
| if (embeddingProviderAvailable !== false) { | ||
| await new Promise((r) => setTimeout(r, TRANSIENT_RETRY_DELAY_MS)); |
There was a problem hiding this comment.
The code uses a Promise-based setTimeout (await new Promise((r) => setTimeout(r, TRANSIENT_RETRY_DELAY_MS))) but the project prefers async/await patterns. While function is not a security issue, it's inconsistent with async best practices.
Suggestion: Use a helper function like const delay = (ms) => new Promise((r) => setTimeout(r, ms)); and then call await delay(TRANSIENT_RETRY_DELAY_MS);
Suggestion:
| if (embeddingProviderAvailable !== false) { | |
| await new Promise((r) => setTimeout(r, TRANSIENT_RETRY_DELAY_MS)); | |
| function delay(ms) { | |
| return new Promise((r) => setTimeout(r, ms)); | |
| } | |
| // In _wrapProvider: | |
| await delay(TRANSIENT_RETRY_DELAY_MS); |
| } else if (agenticResult && agenticResult.isAgentic === false) { | ||
| // The detector already scored this turn NON-agentic using EFFECTIVE | ||
| // tools (harness baseline subtracted, WS3.2). Attached-tool dims must | ||
| // not out-vote that verdict: a bare "Hi" from a harness with 9 default | ||
| // tools is not a tool-orchestration workload. Cap, never raise. | ||
| toolUse = Math.min(toolUse, 0.25); |
There was a problem hiding this comment.
The new else-if branch for non-agentic turns uses a hardcoded threshold (0.25). Consider extracting this to a named constant for better maintainability and easier tuning across the codebase.
Suggestion:
| } else if (agenticResult && agenticResult.isAgentic === false) { | |
| // The detector already scored this turn NON-agentic using EFFECTIVE | |
| // tools (harness baseline subtracted, WS3.2). Attached-tool dims must | |
| // not out-vote that verdict: a bare "Hi" from a harness with 9 default | |
| // tools is not a tool-orchestration workload. Cap, never raise. | |
| toolUse = Math.min(toolUse, 0.25); | |
| const NON_AenticENTIC_CAP = 0.25; | |
| // ... | |
| } else if (agenticResult && agenticResult.isAgentic === false) { | |
| toolUse = Math.min(toolUse, NON_AGENIC_CAP); |
| function stripHarnessEnvelope(text) { | ||
| if (typeof text !== 'string' || text.length === 0) return typeof text === 'string' ? text : ''; | ||
| try { | ||
| if (!text.includes('<')) return text; |
There was a problem hiding this comment.
The stripHarnessEnvelope function uses regex matching on user input without explicit length validation. While the function has a try/catch block and basic type checking, very large inputs could potentially cause performance issues or ReDoS (Regular Expression Denial of Service) vulnerabilities through the multiple regex operations (USER_QUERY_RE, PAIRED_RES, UNCLOSED_RES).
Suggestion:
| function stripHarnessEnvelope(text) { | |
| if (typeof text !== 'string' || text.length === 0) return typeof text === 'string' ? text : ''; | |
| try { | |
| if (!text.includes('<')) return text; | |
| function stripHarnessEnvelope(text) { | |
| if (typeof text !== 'string' || text.length === 0) return typeof text === 'string' ? text : ''; | |
| if (text.length > 10000) return text; // Limit regex processing to prevent ReDoS | |
| try { | |
| if (!text.includes('<')) return text; |
| const _tierLadder = ['SIMPLE', 'MEDIUM', 'COMPLEX', 'REASONING']; | ||
| const _legacyIdx = Math.max(0, _tierLadder.indexOf(tier)); |
There was a problem hiding this comment.
When tier is not found in _tierLadder (e.g., an unknown or future tier value), indexOf returns -1 and Math.max(0, -1) makes _legacyIdx 0. This incorrectly treats unknown tiers as 'SIMPLE', potentially causing unexpected one-band-up caps.
Suggestion:
| const _tierLadder = ['SIMPLE', 'MEDIUM', 'COMPLEX', 'REASONING']; | |
| const _legacyIdx = Math.max(0, _tierLadder.indexOf(tier)); | |
| const _tierLadder = ['SIMPLE', 'MEDIUM', 'COMPLEX', 'REASONING']; | |
| const _legacyIdx = _tierLadder.indexOf(tier); | |
| if (_legacyIdx === -1) { /* unknown tier: skip cap logic or default to lowest */ } |
| toolCall: info.tool_call ?? false, | ||
| reasoning: info.reasoning ?? false, | ||
| vision: Array.isArray(info.input) && info.input.includes('image'), | ||
| vision: Array.isArray(info.modalities?.input) && info.modalities.input.includes('image'), |
There was a problem hiding this comment.
The vision property check on line 287 assumes info.modalities?.input exists, but the fallback info.input may still be used by some model definitions. If modalities is not present but input is, the model will incorrectly have vision:false.
Suggestion:
| vision: Array.isArray(info.modalities?.input) && info.modalities.input.includes('image'), | |
| vision: (Array.isArray(info.modalities?.input) && info.modalities.input.includes('image')) || (Array.isArray(info.input) && info.input.includes('image')), |
|
Re the non-inline |
No description provided.