Codex/claude cli provider

[Yukoval-Dakia]

Description

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[github-actions]

🔍 OpenCodeReview found 44 issue(s) in this PR.

• ✅ 44 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

Minor consistency issue: all other subcommands (review, rules, config, llm) have a corresponding "Use ... for more information" hint at the bottom of the usage text, but learn is missing one. Consider adding:

Use "ocr learn -h" for more information about learn.

[github-actions]

Bug: Silent failure in dedicated ingest command. setupLearnings is designed as a best-effort helper for the review command — it prints warnings to stderr and returns a disabled suppressor on any error, so the review proceeds unaffected. However, for the standalone learn ingest command, the ingest IS the primary operation. Discarding its return value means:

1. If there's no LLM token, the ingest silently skips with only a stderr message, and the command exits successfully (exit code 0).
2. If the store can't be opened or written, same silent success.
3. If embedding fails for all items, same silent success.

In a CI/CD pipeline (the stated use case), the job will appear green even though no learnings were stored. This defeats the purpose of a dedicated ingest entry point.

Consider either:

• Extracting the core ingest logic into a separate function that returns errors, and calling it directly here instead of reusing setupLearnings.
• Or at minimum, checking whether cfg.FeedbackPath is set and the ingest actually ran before returning nil.

[github-actions]

The LlmConfig struct is missing the ClaudeRuntime field that exists in the resolver's llmFileConfig. The resolver (internal/llm/resolver.go) supports claude as a protocol and reads claude_runtime from the config file, but users cannot set it via ocr config set llm.claude_runtime ... because:

1. There is no ClaudeRuntime field in this struct.
2. There is no case "llm.claude_runtime" handler in setConfigValue.
3. The Protocol field validation rejects "claude" (only allows anthropic, openai, codex).
4. The comment on Protocol says // anthropic, openai, or codex but should also include claude.

This means the claude protocol is unreachable through the config CLI, creating an inconsistency with the resolver.

Suggestion:

Suggested change

	Protocol string `json:"protocol,omitempty"` // anthropic, openai, or codex
	CodexRuntime string `json:"codex_runtime,omitempty"` // exec or app_server
	Protocol string `json:"protocol,omitempty"` // anthropic, openai, codex, or claude
	CodexRuntime string `json:"codex_runtime,omitempty"` // exec or app_server
	ClaudeRuntime string `json:"claude_runtime,omitempty"` // exec or app_server

[github-actions]

The error message says the value must be 'exec' or 'app_server', but the switch statement actually accepts four variants: exec, app_server, app-server, and appserver. The error message should reflect all accepted values, or at minimum mention that hyphenated/concatenated forms are also accepted. This avoids confusing users who try app-server and get an error message telling them it's invalid, even though it would work.

Suggestion:

Suggested change

	return fmt.Errorf("invalid llm.codex_runtime value %q: must be 'exec' or 'app_server'", value)
	return fmt.Errorf("invalid llm.codex_runtime value %q: must be 'exec', 'app_server', 'app-server', or 'appserver'", value)

[github-actions]

Invalid environment variable values are silently ignored. If a user sets OCR_MIN_SEVERITY=mnior (typo) or OCR_MIN_CONFIDENCE=abc, the filter falls back to defaults without any warning. Given that the rest of this codebase consistently warns on stderr for configuration issues (e.g., setupLearnings in review_cmd.go logs warnings like [ocr] learnings: ...), this function should follow the same pattern and emit a warning when an env var is set but cannot be parsed. In CI environments, silent fallback to defaults can make users believe their filter is configured when it isn't.

Suggestion:

Suggested change

	if v := strings.TrimSpace(os.Getenv("OCR_MIN_SEVERITY")); v != "" {
	if r := severityRank(v); r > 0 {
	f.minSeverity = r
	f.minSeverityLabel = strings.ToLower(v)
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_CONFIDENCE")); v != "" {
	if c, err := strconv.ParseFloat(v, 64); err == nil && c >= 0 && c <= 1 {
	f.minConfidence = c
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_SEVERITY")); v != "" {
	if r := severityRank(v); r > 0 {
	f.minSeverity = r
	f.minSeverityLabel = strings.ToLower(v)
	} else {
	fmt.Fprintf(os.Stderr, "[ocr] warning: OCR_MIN_SEVERITY=%q is not a valid severity (blocker|major|minor|nit); using default %q\n", v, f.minSeverityLabel)
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_CONFIDENCE")); v != "" {
	if c, err := strconv.ParseFloat(v, 64); err == nil && c >= 0 && c <= 1 {
	f.minConfidence = c
	} else {
	fmt.Fprintf(os.Stderr, "[ocr] warning: OCR_MIN_CONFIDENCE=%q is invalid (must be 0.0-1.0); using default %.2f\n", v, f.minConfidence)
	}
	}

[github-actions]

When OCR_LLM_PROTOCOL is set to openai or anthropic (without URL/token/model env vars), the strategy swap promotes tryOCREnv ahead of tryOCRConfig. Since tryOCREnv returns a hard error (not ok=false) in this fail-fast path, the loop at line 76-78 returns immediately — preventing tryOCRConfig from ever being tried.

This means a user who sets OCR_LLM_PROTOCOL=openai to explicitly select the protocol, while having valid openai-compatible endpoint details in their config file, will get an unhelpful error about missing env vars instead of successfully resolving from the config file.

Consider either:

1. Only swapping strategies when the protocol is codex or claude (which are self-contained), or
2. Having tryOCREnv return (ep, false, nil) instead of an error for openai/anthropic when URL/token/model are missing, so the loop continues to the next strategy.

[github-actions]

The error message only mentions codex/claude as valid values for OCR_LLM_PROTOCOL, but normalizeProtocol also accepts anthropic and openai. Users seeing this error might not realize those are also valid protocol options.

Suggestion:

Suggested change

	return ResolvedEndpoint{}, fmt.Errorf("no valid LLM endpoint configured; set OCR_LLM_URL/OCR_LLM_TOKEN/OCR_LLM_MODEL, ~/.opencodereview/config.json, ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN/ANTHROPIC_MODEL, or OCR_LLM_PROTOCOL=codex/claude")
	return ResolvedEndpoint{}, fmt.Errorf("no valid LLM endpoint configured; set OCR_LLM_URL/OCR_LLM_TOKEN/OCR_LLM_MODEL, ~/.opencodereview/config.json, ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN/ANTHROPIC_MODEL, or OCR_LLM_PROTOCOL=anthropic|openai|codex|claude")

[github-actions]

The retry on errEmptyCodexToolCalls re-invokes toolCompletionByRuntime with the same original ctx. Both the exec path (runCodex) and app-server path (runCodexAppServer) create their own context.WithTimeout(ctx, c.cfg.Timeout) internally, so each attempt gets the full timeout budget. In the worst case, the total wall-clock time can reach 2 × Timeout (e.g., 20 minutes with the default 10-minute timeout).

Consider deriving the retry's context from the first attempt's remaining deadline, or using a shared parent timeout that encompasses both attempts, to bound the total time.

[github-actions]

buildExecArgs appears to be unused — all callers use buildExecArgsForModel directly. Consider removing this method to reduce dead code.

[github-actions]

toolCallChatResponse (singular) appears to be unused — only toolCallsChatResponse (plural) is called. Consider removing this function to reduce dead code.

[github-actions]

Minor consistency issue: all other subcommands (review, rules, config, llm) have a corresponding "Use ... for more information" hint at the bottom of the usage text, but learn is missing one. Consider adding:

Use "ocr learn -h" for more information about learn.

[github-actions]

Bug: Silent failure in dedicated ingest command. setupLearnings is designed as a best-effort helper for the review command — it prints warnings to stderr and returns a disabled suppressor on any error, so the review proceeds unaffected. However, for the standalone learn ingest command, the ingest IS the primary operation. Discarding its return value means:

1. If there's no LLM token, the ingest silently skips with only a stderr message, and the command exits successfully (exit code 0).
2. If the store can't be opened or written, same silent success.
3. If embedding fails for all items, same silent success.

In a CI/CD pipeline (the stated use case), the job will appear green even though no learnings were stored. This defeats the purpose of a dedicated ingest entry point.

Consider either:

• Extracting the core ingest logic into a separate function that returns errors, and calling it directly here instead of reusing setupLearnings.
• Or at minimum, checking whether cfg.FeedbackPath is set and the ingest actually ran before returning nil.

[github-actions]

The LlmConfig struct is missing the ClaudeRuntime field that exists in the resolver's llmFileConfig. The resolver (internal/llm/resolver.go) supports claude as a protocol and reads claude_runtime from the config file, but users cannot set it via ocr config set llm.claude_runtime ... because:

1. There is no ClaudeRuntime field in this struct.
2. There is no case "llm.claude_runtime" handler in setConfigValue.
3. The Protocol field validation rejects "claude" (only allows anthropic, openai, codex).
4. The comment on Protocol says // anthropic, openai, or codex but should also include claude.

This means the claude protocol is unreachable through the config CLI, creating an inconsistency with the resolver.

Suggestion:

Suggested change

	Protocol string `json:"protocol,omitempty"` // anthropic, openai, or codex
	CodexRuntime string `json:"codex_runtime,omitempty"` // exec or app_server
	Protocol string `json:"protocol,omitempty"` // anthropic, openai, codex, or claude
	CodexRuntime string `json:"codex_runtime,omitempty"` // exec or app_server
	ClaudeRuntime string `json:"claude_runtime,omitempty"` // exec or app_server

[github-actions]

The error message says the value must be 'exec' or 'app_server', but the switch statement actually accepts four variants: exec, app_server, app-server, and appserver. The error message should reflect all accepted values, or at minimum mention that hyphenated/concatenated forms are also accepted. This avoids confusing users who try app-server and get an error message telling them it's invalid, even though it would work.

Suggestion:

Suggested change

	return fmt.Errorf("invalid llm.codex_runtime value %q: must be 'exec' or 'app_server'", value)
	return fmt.Errorf("invalid llm.codex_runtime value %q: must be 'exec', 'app_server', 'app-server', or 'appserver'", value)

[github-actions]

Invalid environment variable values are silently ignored. If a user sets OCR_MIN_SEVERITY=mnior (typo) or OCR_MIN_CONFIDENCE=abc, the filter falls back to defaults without any warning. Given that the rest of this codebase consistently warns on stderr for configuration issues (e.g., setupLearnings in review_cmd.go logs warnings like [ocr] learnings: ...), this function should follow the same pattern and emit a warning when an env var is set but cannot be parsed. In CI environments, silent fallback to defaults can make users believe their filter is configured when it isn't.

Suggestion:

Suggested change

	if v := strings.TrimSpace(os.Getenv("OCR_MIN_SEVERITY")); v != "" {
	if r := severityRank(v); r > 0 {
	f.minSeverity = r
	f.minSeverityLabel = strings.ToLower(v)
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_CONFIDENCE")); v != "" {
	if c, err := strconv.ParseFloat(v, 64); err == nil && c >= 0 && c <= 1 {
	f.minConfidence = c
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_SEVERITY")); v != "" {
	if r := severityRank(v); r > 0 {
	f.minSeverity = r
	f.minSeverityLabel = strings.ToLower(v)
	} else {
	fmt.Fprintf(os.Stderr, "[ocr] warning: OCR_MIN_SEVERITY=%q is not a valid severity (blocker|major|minor|nit); using default %q\n", v, f.minSeverityLabel)
	}
	}
	if v := strings.TrimSpace(os.Getenv("OCR_MIN_CONFIDENCE")); v != "" {
	if c, err := strconv.ParseFloat(v, 64); err == nil && c >= 0 && c <= 1 {
	f.minConfidence = c
	} else {
	fmt.Fprintf(os.Stderr, "[ocr] warning: OCR_MIN_CONFIDENCE=%q is invalid (must be 0.0-1.0); using default %.2f\n", v, f.minConfidence)
	}
	}

[github-actions]

Bug: Fork PR refs will fail to resolve. The --to "origin/${HEAD_REF}" argument assumes the head branch exists on origin, but for fork PRs, the head branch only exists on the fork's remote — not on origin. The previous version of this workflow explicitly fetched the PR ref via git fetch origin pull/${PR_NUMBER}/head, which was removed.

This affects both pull_request events from forks (where head.ref is the fork's branch name) and issue_comment events on fork PRs (where head_ref comes from the API).

Consider restoring a fetch step, e.g.:

- name: Fetch PR head ref
  run: git fetch origin pull/${{ github.event.pull_request.number || steps.pr-context.outputs.pr_number }}/head:pr-head

And then use pr-head as the --to argument instead of origin/${HEAD_REF}.

[github-actions]

Security concern: Executing code from the PR branch with access to secrets. This step runs on pull_request events (not pull_request_target), which means the checkout is of the PR's merge commit. An attacker could modify scripts/github-actions/collect-feedback.js in their PR to execute arbitrary code with access to secrets.GITHUB_TOKEN (passed via github-token) and potentially other secrets available in the job environment.

The old workflow used pull_request_target specifically to avoid this class of vulnerability. Consider either:

1. Switching back to pull_request_target (and accepting the trade-offs), or
2. Pinning the script to a trusted ref (e.g., the default branch) rather than loading it from the checked-out workspace, e.g. by fetching the file content via the GitHub API from the base branch.

[github-actions]

Potential issue: Global serialization bottleneck. The concurrency group ocr-review is shared across all PRs in the repository, and cancel-in-progress: false means runs queue indefinitely. For repositories with multiple active PRs, reviews will be serialized and delayed significantly.

The previous per-PR concurrency group (${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}) allowed concurrent reviews across different PRs while still preventing duplicate runs on the same PR. If the intent is to protect a single self-hosted runner, consider using a matrix-level concurrency or a runner-group approach instead of serializing all reviews globally.

[github-actions]

Performance: Sequential embedding API calls per comment. Each comment triggers a separate Embed network call (30s timeout configured in BigModelEmbedder). For a review with many comments, this adds significant latency (N × ~200-500ms typical embedding API latency). Consider either:

1. Adding a BatchEmbed(ctx, texts []string) method to the Embedder interface to amortize round-trips, or
2. Using bounded concurrency (e.g., errgroup with a semaphore) to parallelize embedding calls.

Additionally, BestRejected performs a linear scan over all rejected entries for each comment, making the overall complexity O(N×M) where N=comments and M=rejected learnings. This is fine for small stores but worth noting as the store grows.

[github-actions]

Robustness: Path comparison may miss equivalent paths. Simple string equality after trimming whitespace won't match paths that differ in normalization (e.g., ./src/foo.go vs src/foo.go, or dir/../dir/foo.go vs dir/foo.go). Consider using filepath.Clean on both sides before comparison to avoid missed suppressions due to path formatting differences:

func pathMatches(stored, current string) bool {
    stored = strings.TrimSpace(stored)
    if stored == "" {
        return true
    }
    return filepath.Clean(stored) == filepath.Clean(strings.TrimSpace(current))
}

[github-actions]

Potential false positives from name-only matching: The References method matches any *ast.Ident whose Name equals the target, without checking whether the identifier actually refers to the same symbol. This means local variables, function parameters, struct field names in composite literals, or other unrelated identifiers that happen to share the same name will be incorrectly reported as references.

For example, if the changed symbol is Config (a package-level type), and another file has func foo(Config string) { ... }, the parameter Config would be falsely reported as a reference.

Since this is used for impact analysis to guide code review, false positives could mislead reviewers. Consider at minimum documenting this limitation in the method or interface comment, or exploring lightweight heuristics (e.g., skipping identifiers that are themselves declarations via id.Obj != nil && id.Obj.Pos() == id.Pos()) to reduce noise.

[github-actions]

Bug: Lines starting with \ (e.g., \ No newline at end of file) are a standard part of unified diffs and should not increment the new-file line counter. Currently they fall into the default case and cause newLine++, which shifts all subsequent line numbers by 1. This can lead to incorrect changed-line detection and wrong symbols being flagged in impact analysis.

Add an explicit case to skip these metadata lines:

case strings.HasPrefix(line, "\\"):
    // diff metadata (e.g., "\ No newline at end of file"); skip

Note: The test suite does not cover this edge case either.

Suggestion:

Suggested change

	default:
	newLine++ // context line
	case strings.HasPrefix(line, "\\"):
	// diff metadata (e.g., "\ No newline at end of file"); skip
	default:
	newLine++ // context line

[github-actions]

Missing stderr capture makes node failures opaque. When cmd.Output() fails (e.g., node exits non-zero because TypeScript is not installed, or the script has a syntax error), the returned *exec.ExitError does not include stderr by default. The caller receives only an exit status like exit status 1 with no diagnostic from the Node process. This will be especially confusing when TypeScript is missing from the repo's node_modules.

Capture stderr and include it in the error message so failures are actionable:

Suggestion:

Suggested change

	cmd.Stdin = strings.NewReader(string(in))
	out, err := cmd.Output()
	if err != nil {
	return resp, err
	}
	cmd.Stdin = strings.NewReader(string(in))
	var stderr bytes.Buffer
	cmd.Stderr = &stderr
	out, err := cmd.Output()
	if err != nil {
	return resp, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String()))
	}

[github-actions]

nodeHasTypeScript is defined but never called. This method exists to check whether TypeScript is available, but no caller invokes it before runTSHelper. When TypeScript is not installed in the target repo, runTSHelper will fail with an unhelpful error from node (especially given the missing stderr capture above). Either integrate this check into runTSHelper / the analyzer selection logic, or remove the dead code to avoid confusion.

[github-actions]

The nearest-rank percentile method produces degenerate results for small sample sizes. For example, with 3 rejected learnings (3 pairs), both P90 and P95 map to index int(0.90 * 2) = 1 and int(0.95 * 2) = 1 respectively — returning the same value as the median. With 2 rejected learnings (1 pair), all percentiles return the single value.

While the Suggested threshold is clamped to mitigate this, the reported P90/P95 stats themselves can be misleading to a user trying to understand the distribution. Consider either:

1. Using linear interpolation between adjacent ranks (e.g., the "exclusive" or "inclusive" percentile method), or
2. Documenting that these statistics are only meaningful above a minimum pair count (e.g., ≥10 pairs) and returning ok=false below that threshold.

[github-actions]

Bug: The kindFor function does not handle VariableDeclaration nodes (const/let/var). The Symbol.Kind contract in analyzer.go explicitly lists const as an expected kind, and the Go analyzer (go_analyzer.go) captures const and var declarations. This means that if a changed line contains only variable declarations (e.g., export const FOO = ...), no symbols will be reported, causing the impact analyzer to miss those changes entirely.

Add handling for variable declarations, e.g.:

if (ts.isVariableDeclaration(n)) return 'const'; // or distinguish via parent flags

Suggestion:

Suggested change

	const kindFor = n => {
	if (ts.isFunctionDeclaration(n)) return 'function';
	if (ts.isMethodDeclaration(n)) return 'method';
	if (ts.isClassDeclaration(n)) return 'class';
	if (ts.isInterfaceDeclaration(n)) return 'interface';
	if (ts.isTypeAliasDeclaration(n)) return 'type';
	if (ts.isEnumDeclaration(n)) return 'enum';
	return null;
	};
	const kindFor = n => {
	if (ts.isFunctionDeclaration(n)) return 'function';
	if (ts.isMethodDeclaration(n)) return 'method';
	if (ts.isClassDeclaration(n)) return 'class';
	if (ts.isInterfaceDeclaration(n)) return 'interface';
	if (ts.isTypeAliasDeclaration(n)) return 'type';
	if (ts.isEnumDeclaration(n)) return 'enum';
	if (ts.isVariableDeclaration(n)) {
	const p = n.parent;
	if (p && ts.isVariableDeclarationList(p) && (p.flags & ts.NodeFlags.Const)) return 'const';
	return 'var';
	}
	return null;
	};

[github-actions]

Potential accuracy issue: References are matched purely by identifier text (n.text === req.name) without semantic symbol resolution. This means identically-named but unrelated identifiers (e.g., a local variable shadowing an import, different class members with the same name) will be incorrectly reported as references. While the upstream git grep -w pre-filter reduces noise, this can still produce false positives that mislead reviewers.

Consider using ts.createProgram + TypeChecker.getSymbolAtLocation() for proper symbol-level resolution when precision matters. If this is intentionally a best-effort heuristic, adding a comment documenting this limitation would help future maintainers.

[github-actions]

Bug: Codex/Claude protocol users can never use the learnings feature.

When the resolved protocol is codex or claude, the resolver returns a ResolvedEndpoint with an empty Token field (these protocols use their own authentication mechanisms). Since setupLearnings receives ep.Token and immediately returns a disabled suppressor when it's empty (line 325-327), the learnings/feedback loop is permanently unavailable for codex/claude users.

Consider either:

1. Reading the embedding API token from a dedicated env var (e.g., OCR_EMBED_TOKEN) independent of the LLM endpoint token, or
2. Falling back to a separate token source when ep.Token is empty.