From 1e43d37b6cc8f7f14b55473569dd95efcd4d7f5d Mon Sep 17 00:00:00 2001 From: mikkeldanielsen <60819393+mikkeldanielsen@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:34:26 +0200 Subject: [PATCH 1/7] feat(rules): add PHP and Composer rules (#603) --- internal/config/allowlist/allowed_ext_test.go | 4 ++ .../allowlist/supported_file_types.json | 1 + .../config/rules/rule_docs/composer_json.md | 39 +++++++++++ internal/config/rules/rule_docs/php.md | 62 ++++++++++++++++++ internal/config/rules/system_rules.json | 2 + internal/config/rules/system_rules_test.go | 65 +++++++++++++++++++ pages/src/content/docs/en/review-rules.md | 5 +- pages/src/content/docs/ja/review-rules.md | 4 +- pages/src/content/docs/zh/review-rules.md | 4 +- 9 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 internal/config/rules/rule_docs/composer_json.md create mode 100644 internal/config/rules/rule_docs/php.md diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index 111d3bb4..fa153d5f 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -17,6 +17,10 @@ func TestIsAllowedExt(t *testing.T) { {".astro", true}, {".ASTRO", true}, {".py", true}, + {".php", true}, + {".PHP", true}, + {".phtml", true}, + {".PHTML", true}, {".rs", true}, {".ets", true}, {".ETS", true}, diff --git a/internal/config/allowlist/supported_file_types.json b/internal/config/allowlist/supported_file_types.json index 430e135e..be73b443 100644 --- a/internal/config/allowlist/supported_file_types.json +++ b/internal/config/allowlist/supported_file_types.json @@ -28,6 +28,7 @@ ".rake", ".gemspec", ".php", + ".phtml", ".swift", ".m", ".mm", diff --git a/internal/config/rules/rule_docs/composer_json.md b/internal/config/rules/rule_docs/composer_json.md new file mode 100644 index 00000000..20747f15 --- /dev/null +++ b/internal/config/rules/rule_docs/composer_json.md @@ -0,0 +1,39 @@ +#### Composer Manifest Review Principles +> Focus on newly introduced correctness, reproducibility, security, and deployment defects. Inspect source usage, CI, containers, deployment configuration, and nearby workspace manifests before claiming a dependency or platform incompatibility. Do not turn preferences about exact pins versus compatible ranges into findings. + +#### Dependency Constraints and Resolution +- Wildcard constraints such as `*`, unconstrained `dev-*` branches, or mutable VCS references introduced without a committed, current lock file where application builds must be reproducible, or in a reusable library where consumers resolve dependencies themselves. Compatible version ranges are normal for libraries and should not be flagged by default. +- A changed constraint that unintentionally permits an incompatible major version, excludes the repository's supported range, or conflicts with another direct requirement. +- The same package declared inconsistently across `require` and `require-dev`, or a production package available only through development dependencies. +- A newly used package or mandatory PHP extension absent from `require`, causing clean production installs to fail. +- Do not report a known vulnerability without reliable advisory evidence applicable to the resolved version range. + +#### PHP and Platform Compatibility +- The `php` constraint contradicts syntax or APIs used by the changed code, the framework's supported range, or the runtime configured in CI and deployment. +- A required native extension missing from `ext-*` requirements, or an extension requirement made mandatory even though the code has a working optional fallback. +- `config.platform` masking a runtime or extension mismatch that will occur in production. Confirm the actual deployment platform before reporting. +- Composer or plugin API requirements incompatible with the Composer version used by CI, containers, or release tooling. + +#### Autoloading and Package Layout +- Incorrect PSR-4 namespace prefixes or paths, overlapping prefixes that resolve the wrong class, or moved classes left unreachable by autoload configuration. +- Production classes placed only in `autoload-dev`, or test-only helpers exposed through production autoloading when that changes packaged behavior. +- `autoload.files` additions that execute side effects on every Composer bootstrap or rely on an unsafe initialization order. +- Classmap, exclusion, or files entries left stale after directories are moved or renamed. + +#### Scripts and Plugin Execution +- Lifecycle scripts that run destructive commands, interpolate untrusted environment values into a shell, require interactive input in CI, or invoke tools not available from declared dependencies. +- Composer scripts that recursively invoke Composer or make production installation depend on development-only packages or local state. +- A newly required Composer plugin without an intentional `config.allow-plugins` decision, or wildcard/broad authorization that permits unexpected plugin code to execute during install or update. +- Do not flag scripts or plugins solely because they execute code; establish a concrete unsafe command, trust-boundary change, or installation failure. + +#### Repositories and Supply Chain +- `secure-http` disabled, plaintext repository URLs, embedded credentials, or newly introduced package sources without appropriate integrity and access controls. +- Repository priority or canonical settings that can cause a private/public package to resolve from an unintended source. +- `package` or VCS repositories pointing to mutable or unverifiable artifacts where reproducible source selection is required. +- Secrets, tokens, or private repository credentials exposed in committed manifest data. Report an internal URL only when the manifest is publicly distributed and the URL itself reveals sensitive infrastructure information. + +#### Stability, Package Semantics, and Release Metadata +- `minimum-stability` weakened so unrelated development packages can enter resolution, especially without `prefer-stable`; verify whether a narrowly constrained development dependency would suffice. +- Incorrect `replace`, `provide`, or `conflict` declarations that can make Composer omit a required implementation or accept an incompatible package. +- Changes to `type`, `bin`, installer paths, archive include/exclude rules, or framework `extra` metadata that break installation or packaging. +- Published packages missing or invalid required metadata only when the repository is actually distributed as a package; do not apply publishing requirements to private applications. diff --git a/internal/config/rules/rule_docs/php.md b/internal/config/rules/rule_docs/php.md new file mode 100644 index 00000000..c7775c89 --- /dev/null +++ b/internal/config/rules/rule_docs/php.md @@ -0,0 +1,62 @@ +#### PHP Review Principles +> Favor precision over recall: report only defects that are likely real in the changed code and its reachable context. Treat correctness and security findings as blocking; style-only suggestions are non-blocking. Account for the project's PHP version and framework conventions before reporting version- or lifecycle-dependent behavior. + +Before making a non-local claim, use `file_read` and `code_search` to verify callers, input sources, framework configuration, template context, and resource ownership. Do not duplicate findings reliably enforced by PHPStan, Psalm, PHP_CodeSniffer, the formatter, or the PHP compiler unless the diff demonstrates a concrete consequence those tools do not express. + +#### Type Juggling, Equality, and Null Semantics +- Loose comparison (`==` or `!=`) whose coercion can make distinct security- or domain-sensitive values compare equal. Prefer strict comparison when operands are expected to have the same type; do not flag deliberate, validated normalization. +- Truthiness or `empty()` checks that incorrectly treat `0`, `"0"`, `false`, `null`, and an empty value as equivalent when those states have different meanings. +- `isset()` used when a present key with a `null` value must be distinguished from a missing key; use `array_key_exists()` when presence, rather than non-nullness, is the contract. +- Nullable, union, or `false`-returning APIs whose failure value reaches code that assumes a usable object, scalar, or resource. Confirm the declared and runtime contract before flagging. +- Numeric-string, arithmetic, or comparison behavior that depends on a different PHP version from the one supported by `composer.json`, CI, or deployment configuration. + +#### Arrays, Iteration, and Value Semantics +- Array keys read without handling a reachable missing-key path, especially request data, decoded JSON, database rows, or optional configuration. +- A `foreach` value variable iterated by reference and then reused without `unset()`, leaving it aliased to the final element and allowing later assignments to corrupt the array. +- Array union (`+`), `array_merge`, spread syntax, or numeric-key reindexing used with semantics different from the intended overwrite and ordering behavior. +- Callbacks or closures that capture a loop variable by reference and later observe an unintended final or mutated value. +- Mutation during iteration that can skip, duplicate, or unexpectedly retain elements. Do not flag mutation whose traversal behavior is deliberate and locally evident. + +#### Errors, Exceptions, and API Contracts +- `Throwable` or `Exception` caught and silently discarded, converted into success, or replaced with a misleading default on a path where the failure matters. +- Catching a broad exception around unrelated operations so the handler cannot distinguish the expected failure from a programming or infrastructure defect. +- A codebase contract inconsistently mixing exceptions, `false`, and `null` for the same failure, causing callers to miss an error path. +- Cleanup, rollback, or response-finalization code that hides the primary exception or returns success after the operation failed. +- Warnings or errors suppressed with `@` where suppression can turn a meaningful failure into invalid state. Do not flag a narrowly documented compatibility probe that checks the result safely. + +#### Resources, Transactions, and Request Lifecycle +- Transactions, locks, database cursors, or resources in long-running processes not released, committed, or rolled back on every reachable path when delayed cleanup can exhaust capacity or break correctness. Do not flag ordinary request-scoped streams or files merely because PHP can release them at request shutdown. +- Database transactions with early returns or exception paths that can leave the transaction open, or nested transaction assumptions unsupported by the active driver/framework. +- cURL or stream operations lacking timeouts on a request or worker path where a remote endpoint can stall execution. +- Session locks held across slow network, database, or CPU work when concurrent requests for the same session must proceed. +- Do not report resources owned by a framework, dependency-injection container, generator consumer, or caller when ownership transfer is established by the surrounding code. + +#### Database and ORM Correctness +- SQL assembled from untrusted values instead of parameter binding. Identifiers such as column names and sort directions cannot usually be bound and require an allowlist. +- Raw ORM expressions, query fragments, or dynamic table/column names that bypass the framework's normal parameterization with attacker-controlled data. +- Missing transaction boundaries when a changed multi-step write must be atomic, or side effects ordered so a rollback cannot restore consistency. +- N+1 queries or repeated remote calls only when the loop is reachable at meaningful scale and eager loading or batching preserves behavior. +- Mass-assignment exposure only when request-controlled fields reach a model and the framework's fillable/guarded/schema configuration does not already constrain them. + +#### Web and Template Security Boundaries +Confirm attacker control and the output or execution context before reporting. Framework validation and auto-escaping may make an otherwise dangerous-looking operation safe. + +- Untrusted output rendered without context-appropriate escaping for HTML text, attributes, URLs, JavaScript, or CSS. For `.phtml` templates, verify whether the view helper already escapes the value and whether raw HTML is intentional and sanitized. +- Authorization enforced only in a client, template, or hidden control rather than at the server-side operation; check route middleware, policies, voters, and controller guards before flagging. +- State-changing browser requests missing required CSRF protection when cookie-based authentication makes cross-site invocation possible. Do not flag token-authenticated APIs that are not vulnerable to ambient credentials. +- Redirects, response headers, or cookies built from untrusted data without validation or appropriate `Secure`, `HttpOnly`, and `SameSite` protections where those properties are required. +- File uploads trusted by client filename, extension, or MIME header alone; verify server-side type checks, generated storage names, destination boundaries, and executable-file handling. +- User-controlled paths used for filesystem access without normalization and boundary enforcement, enabling traversal, symlink escape, or unintended overwrite. + +#### Dynamic Execution, Deserialization, and Outbound Requests +- `eval`, dynamic `include`/`require`, variable function calls, reflection, or shell commands reached by untrusted input without a strict allowlist. +- `unserialize()` on attacker-controlled data, including signed data where key management or verification is absent. Prefer a non-executable format; `allowed_classes` reduces object injection but does not make arbitrary data trustworthy. +- Shell commands built through concatenation or incomplete escaping. Prefer direct process APIs with separate arguments and validate option-like attacker-controlled values. +- Outbound URLs derived from untrusted input without required scheme, host, port, redirect, and private-network restrictions, enabling SSRF or credential forwarding. +- Weak randomness or password handling: predictable token generation, reversible password storage, manual password hashing, or non-constant-time comparison of secrets. Prefer `random_bytes`, `password_hash`, `password_verify`, and `hash_equals` as appropriate. +- Secrets, session identifiers, authorization headers, passwords, private keys, or sensitive personal data logged, returned in errors, or embedded in source. + +#### Performance and Review Scope +- Report performance issues only with evidence of meaningful data scale or a hot path: repeated queries, accidental full-result materialization, quadratic array operations, or expensive work repeated inside a loop. +- Suggest tests only for concrete changed failure modes involving coercion, boundary values, errors, transactions, authorization, escaping, or framework configuration. +- Do not make formatting, naming, import ordering, modern-syntax preferences, or advice already enforced by deterministic PHP tooling into blocking findings. diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index 0b599b46..98b6c262 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -7,6 +7,7 @@ "**/build.gradle": "build_gradle.md", "**/package.json": "package_json.md", "**/Cargo.toml": "cargo_toml.md", + "**/composer.json": "composer_json.md", "**/*.{json,json5}": "json.md", ".github/workflows/**/*.{yaml,yml}": "github_workflows.md", ".github/**/*.{yaml,yml}": "github_config.md", @@ -22,6 +23,7 @@ "**/*.{cpp,cc,hpp}": "cpp.md", "**/*.c": "c.md", "**/*.py": "python.md", + "**/*.{php,phtml}": "php.md", "**/*.po": "po.md", "**/*.pot": "pot.md", "**/*.{graphql,gql}": "graphql.md", diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index ed7bc926..60a77881 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -69,6 +69,8 @@ func TestResolve_DefaultRules(t *testing.T) { {"submodule/pom.xml", "snapshot"}, {"src/main/resources/application.properties", "Configuration Error Detection"}, {"frontend/package.json", "latest"}, + {"composer.json", "Composer Manifest Review Principles"}, + {"packages/library/composer.json", "Dependency Constraints and Resolution"}, {"config/app.yaml", "yaml-key"}, {"deploy/values.yml", "yaml-key"}, {"src/pages/index.astro", "client:*"}, @@ -86,6 +88,8 @@ func TestResolve_DefaultRules(t *testing.T) { {"crates/service/Cargo.toml", "Cargo Manifest Hygiene"}, {"scripts/deploy.py", "Mutable Default Arguments"}, {"src/app/main.py", "Mutable Default Arguments"}, + {"public/index.php", "PHP Review Principles"}, + {"templates/account/profile.phtml", "Web and Template Security Boundaries"}, {"locale/zh_CN/LC_MESSAGES/messages.po", "Placeholder Mismatch"}, {"i18n/app.po", "Plural Forms"}, {"locale/messages.pot", "Placeholder Consistency"}, @@ -879,6 +883,67 @@ func TestResolveDetail_SystemGoPatternMatch(t *testing.T) { } } +func TestResolveDetail_SystemPHPPatternMatch(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + resolver, _, err := NewResolver(t.TempDir(), "") + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + dr := resolver.(DetailResolver) + + for _, path := range []string{"index.php", "src/Controller/UserController.php", "TEMPLATES/INDEX.PHTML"} { + t.Run(path, func(t *testing.T) { + detail := dr.ResolveDetail(path) + if detail.Source != "system" { + t.Errorf("expected source 'system', got %q", detail.Source) + } + if detail.Pattern != "**/*.{php,phtml}" { + t.Errorf("expected pattern '**/*.{php,phtml}', got %q", detail.Pattern) + } + for _, required := range []string{ + "PHP Review Principles", + "foreach` value variable iterated by reference", + "unserialize()", + "PHPStan", + } { + if !strings.Contains(detail.Rule, required) { + t.Errorf("expected PHP rule to contain %q", required) + } + } + }) + } +} + +func TestResolveDetail_SystemComposerPatternPrecedesJSON(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + resolver, _, err := NewResolver(t.TempDir(), "") + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + dr := resolver.(DetailResolver) + + for _, path := range []string{"composer.json", "packages/library/composer.json", "PACKAGES/APP/COMPOSER.JSON"} { + t.Run(path, func(t *testing.T) { + detail := dr.ResolveDetail(path) + if detail.Source != "system" { + t.Errorf("expected source 'system', got %q", detail.Source) + } + if detail.Pattern != "**/composer.json" { + t.Errorf("expected pattern '**/composer.json', got %q", detail.Pattern) + } + for _, required := range []string{ + "Composer Manifest Review Principles", + "config.allow-plugins", + "PSR-4", + } { + if !strings.Contains(detail.Rule, required) { + t.Errorf("expected Composer rule to contain %q", required) + } + } + }) + } +} + func TestResolveDetail_ProjectOverridesSystem(t *testing.T) { t.Setenv("HOME", t.TempDir()) dir := t.TempDir() diff --git a/pages/src/content/docs/en/review-rules.md b/pages/src/content/docs/en/review-rules.md index 0da420c0..647b8445 100644 --- a/pages/src/content/docs/en/review-rules.md +++ b/pages/src/content/docs/en/review-rules.md @@ -141,7 +141,8 @@ text the agent should follow: 3. Try `~/.opencodereview/rule.json` in declaration order. 4. Fall back to the embedded system rule layer. -The embedded `system_rules.json` ships with these patterns (in order): +Selected embedded `system_rules.json` patterns are shown below in relative +matching order: | Pattern | Rule doc | |---|---| @@ -151,6 +152,7 @@ The embedded `system_rules.json` ships with these patterns (in order): | `**/build.gradle` | `build_gradle.md` — Gradle dependencies. | | `**/package.json` | `package_json.md` — NPM dependencies / scripts. | | `**/Cargo.toml` | `cargo_toml.md` — Rust manifest. | +| `**/composer.json` | `composer_json.md` — Composer dependencies, autoloading, scripts, plugins, and package configuration. | | `**/*.{json,json5}` | `json.md` — generic JSON (also matches `.json5`). | | `.github/workflows/**/*.{yaml,yml}` | `github_workflows.md` — GitHub Actions workflow YAML. | | `.github/**/*.{yaml,yml}` | `github_config.md` — other `.github` config YAML. | @@ -163,6 +165,7 @@ The embedded `system_rules.json` ships with these patterns (in order): | `**/*.rs` | `rust.md` | | `**/*.{cpp,cc,hpp}` | `cpp.md` | | `**/*.c` | `c.md` | +| `**/*.{php,phtml}` | `php.md` — PHP source and PHP templates. | | *(fallback)* | `default.md` | The resolved rule body becomes the `{{system_rule}}` placeholder in the diff --git a/pages/src/content/docs/ja/review-rules.md b/pages/src/content/docs/ja/review-rules.md index 56a4110e..78984511 100644 --- a/pages/src/content/docs/ja/review-rules.md +++ b/pages/src/content/docs/ja/review-rules.md @@ -104,7 +104,7 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest 3. 宣言順に `~/.opencodereview/rule.json` を試します。 4. 埋め込みのシステムルール層にフォールバックします。 -埋め込みの `system_rules.json` には次のパターンが同梱されています（順序どおり）: +埋め込みの `system_rules.json` から主なパターンを相対的なマッチ順で示します: | パターン | ルールドキュメント | |---|---| @@ -114,6 +114,7 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/build.gradle` | `build_gradle.md`: Gradle 依存関係。 | | `**/package.json` | `package_json.md`: NPM 依存関係 / スクリプト。 | | `**/Cargo.toml` | `cargo_toml.md`: Rust manifest。 | +| `**/composer.json` | `composer_json.md`: Composer の依存関係、自動読み込み、スクリプト、プラグイン、パッケージ設定。 | | `**/*.{json,json5}` | `json.md`: 汎用 JSON（`.json5` にも一致）。 | | `.github/workflows/**/*.{yaml,yml}` | `github_workflows.md`: GitHub Actions ワークフロー YAML。 | | `.github/**/*.{yaml,yml}` | `github_config.md`: その他の `.github` 設定 YAML。 | @@ -126,6 +127,7 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.rs` | `rust.md` | | `**/*.{cpp,cc,hpp}` | `cpp.md` | | `**/*.c` | `c.md` | +| `**/*.{php,phtml}` | `php.md`: PHP ソースと PHP テンプレート。 | | *(fallback)* | `default.md` | 解決されたルール本文は、plan および main task prompt 内の `{{system_rule}}` プレースホルダーの内容になります。 diff --git a/pages/src/content/docs/zh/review-rules.md b/pages/src/content/docs/zh/review-rules.md index 12bed5e1..6c95d4d2 100644 --- a/pages/src/content/docs/zh/review-rules.md +++ b/pages/src/content/docs/zh/review-rules.md @@ -125,7 +125,7 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest 3. 按声明顺序试 `~/.opencodereview/rule.json`。 4. 回退到内嵌系统规则层。 -内嵌 `system_rules.json` 自带这些模式（按序）： +以下是内嵌 `system_rules.json` 的部分模式，按相对匹配顺序排列： | 模式 | 规则文档 | |---|---| @@ -135,6 +135,7 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/build.gradle` | `build_gradle.md`——Gradle 依赖。 | | `**/package.json` | `package_json.md`——NPM 依赖 / 脚本。 | | `**/Cargo.toml` | `cargo_toml.md`——Rust manifest。 | +| `**/composer.json` | `composer_json.md`——Composer 依赖、自动加载、脚本、插件和包配置。 | | `**/*.{json,json5}` | `json.md`——通用 JSON（也匹配 `.json5`）。 | | `.github/workflows/**/*.{yaml,yml}` | `github_workflows.md`——GitHub Actions 工作流 YAML。 | | `.github/**/*.{yaml,yml}` | `github_config.md`——其他 `.github` 配置 YAML。 | @@ -147,6 +148,7 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.rs` | `rust.md` | | `**/*.{cpp,cc,hpp}` | `cpp.md` | | `**/*.c` | `c.md` | +| `**/*.{php,phtml}` | `php.md`——PHP 源代码和 PHP 模板。 | | *(fallback)* | `default.md` | 解析出的规则正文成为 plan 和 main task prompt 中 `{{system_rule}}` 占位符的内容。 From 74afd9c89c69e92fe0f28d1dc8bba181a323fed7 Mon Sep 17 00:00:00 2001 From: chethanuk Date: Thu, 30 Jul 2026 13:01:11 +0400 Subject: [PATCH 2/7] feat(allowlist): add Protocol Buffers (.proto) support (#473) Allow .proto files through the extension allowlist and map them to a dedicated protobuf review rule focused on wire compatibility. Part of #470. --- internal/config/allowlist/allowed_ext_test.go | 2 + .../allowlist/supported_file_types.json | 3 +- internal/config/rules/rule_docs/protobuf.md | 39 +++++++++++++++++++ internal/config/rules/system_rules.json | 1 + internal/config/rules/system_rules_test.go | 2 + 5 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 internal/config/rules/rule_docs/protobuf.md diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index fa153d5f..aea7132b 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -46,6 +46,8 @@ func TestIsAllowedExt(t *testing.T) { {".TFVARS", true}, {".bicep", true}, {".BICEP", true}, + {".proto", true}, + {".PROTO", true}, {".txt", false}, {".md", false}, {".png", false}, diff --git a/internal/config/allowlist/supported_file_types.json b/internal/config/allowlist/supported_file_types.json index be73b443..c976805d 100644 --- a/internal/config/allowlist/supported_file_types.json +++ b/internal/config/allowlist/supported_file_types.json @@ -77,5 +77,6 @@ ".jl", ".hcl", ".tfvars", - ".bicep" + ".bicep", + ".proto" ] diff --git a/internal/config/rules/rule_docs/protobuf.md b/internal/config/rules/rule_docs/protobuf.md new file mode 100644 index 00000000..5d06a342 --- /dev/null +++ b/internal/config/rules/rule_docs/protobuf.md @@ -0,0 +1,39 @@ +> Favor precision over recall: only raise an issue when you are confident it is a real defect, and stay silent when the surrounding context is unclear — a false alarm costs more reviewer trust than a missed minor issue. Treat security and correctness findings as blocking, and style or idiom suggestions as non-blocking. + +#### Obvious Typos or Spelling Errors +- Spelling errors in message, field, enum, enum-value, service, or rpc names at their declaration sites; do not report spelling errors at reference sites +- Comments or option strings with spelling errors that affect readability of the public API surface + +#### Field Numbers and Wire Compatibility +- Reused or renumbered field tags that break existing clients or servers (Wire Compatibility) +- Changing a field's type, label (`optional`/`repeated`/`required`), or oneof membership in a way that breaks wire or JSON compatibility +- Deleting a field without adding both its number and name to `reserved` +- Renaming a field without `json_name` consideration when JSON clients depend on the old name +- Do not flag purely additive new fields with fresh numbers, or documentation-only comment changes + +#### Message and Field Design +- Missing `optional` (proto3) where absence must be distinguishable from the zero value +- `map` used where order matters, or `repeated` used where key lookup would be clearer +- oneof fields that leave an invalid zero-state representable when an explicit sentinel was intended +- Nested messages that re-encode the same domain concept already modeled elsewhere in the package +- Do not report stylistic preference for `message` vs `group` (groups are legacy) when the schema is already consistent + +#### Enums and Defaults +- First enum value is not a zero `*_UNSPECIFIED` (or equivalent) sentinel +- Relying on implicit zero defaults across schema versions when clients treat zero as meaningful data +- Inserting new enum values in the middle of an existing numeric range used by older clients +- Do not flag additive enum values appended at the end with new numbers + +#### Services and RPC Design +- Non-idempotent methods modeled as if they were safe to retry without client-visible side effects +- Multiple rpcs sharing the same request or response message type when distinct contracts would prevent accidental field coupling +- Unbounded client/server streaming without documented flow control, page size, or deadline expectations +- Missing request or response message wrappers that force primitive/scalar request bodies +- Do not flag standard google.api annotations or well-known types used correctly + +#### Security and Resource Limits +- `google.protobuf.Any` accepted from untrusted input without type allowlisting +- Unbounded `repeated`/`map` fields or recursive message depth on untrusted payloads with no application-level limits +- Secrets, tokens, or credentials embedded in field defaults, examples, or comments +- File paths, URLs, or SQL fragments carried as unconstrained strings without validation guidance at the service boundary +- Do not report when limits are enforced outside the schema and that boundary is clearly documented diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index 98b6c262..0dd10066 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -24,6 +24,7 @@ "**/*.c": "c.md", "**/*.py": "python.md", "**/*.{php,phtml}": "php.md", + "**/*.proto": "protobuf.md", "**/*.po": "po.md", "**/*.pot": "pot.md", "**/*.{graphql,gql}": "graphql.md", diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 60a77881..10aca01b 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -102,6 +102,8 @@ func TestResolve_DefaultRules(t *testing.T) { {"modules/network/vpc.hcl", "Overly Permissive Access"}, {"envs/prod.tfvars", "Hardcoded Secrets"}, {"infra/main.bicep", "Hardcoded Secrets"}, + {"api/v1/user.proto", "Wire Compatibility"}, + {"service.proto", "Wire Compatibility"}, } for _, tt := range tests { From 5a7a25dbacf05dd559b89e658647f556c66cc79f Mon Sep 17 00:00:00 2001 From: Abdul Moiz Hussain Date: Thu, 30 Jul 2026 14:10:05 +0500 Subject: [PATCH 3/7] fix(config): warn when active provider shadows llm settings (#588) * fix(config): warn when active provider shadows llm settings * fix(config): suggest correct path for custom provider settings * fix(config): refine shadow warning and unset help --- cmd/opencodereview/config_cmd.go | 48 ++++++++++-- cmd/opencodereview/config_cmd_test.go | 109 +++++++++++++++++++++++++- cmd/opencodereview/flags.go | 6 +- cmd/opencodereview/flags_test.go | 6 ++ 4 files changed, 159 insertions(+), 10 deletions(-) diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 64d19190..3044c03f 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -90,28 +90,64 @@ func runConfigSet(key, value string) error { displayValue = maskKey(value) } fmt.Printf("Set %s = %s\n", key, displayValue) + if warning := legacyLLMShadowWarning(cfg.Provider, key); warning != "" { + fmt.Fprint(os.Stderr, warning) + } return nil } func runConfigUnset(key string) error { - parts := strings.SplitN(key, ".", 2) - if len(parts) != 2 || parts[1] == "" { - return fmt.Errorf("unset supports custom_providers. and mcp_servers.") - } - configPath, err := defaultConfigPath() if err != nil { return err } + if key == "provider" { + return unsetActiveProvider(configPath) + } + + parts := strings.SplitN(key, ".", 2) + if len(parts) != 2 || parts[1] == "" { + return fmt.Errorf("unset supports provider, custom_providers., and mcp_servers.") + } + switch parts[0] { case "custom_providers": return unsetCustomProvider(configPath, parts[1]) case "mcp_servers": return unsetMCPServer(configPath, parts[1]) default: - return fmt.Errorf("unset supports custom_providers. and mcp_servers.") + return fmt.Errorf("unset supports provider, custom_providers., and mcp_servers.") + } +} + +func unsetActiveProvider(configPath string) error { + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + return fmt.Errorf("load config: %w", err) + } + + cfg.Provider = "" + cfg.Model = "" + if err := saveConfig(configPath, cfg); err != nil { + return err + } + + fmt.Println("Cleared active provider and model.") + return nil +} + +func legacyLLMShadowWarning(provider, key string) string { + if provider == "" || !strings.HasPrefix(key, "llm.") { + return "" + } + section := "custom_providers" + if _, isPreset := llm.LookupProvider(provider); isPreset { + section = "providers" } + return fmt.Sprintf("[ocr] WARNING: provider %q is active and takes precedence over llm.* settings.\n"+ + "[ocr] Use 'ocr config set %s.%s. ' to configure the active provider,\n"+ + "[ocr] or run 'ocr config unset provider' to disable provider-based config.\n", provider, section, provider) } func unsetCustomProvider(configPath, name string) error { diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 1cba3f99..f2b4b246 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1,8 +1,10 @@ package main import ( + "io" "os" "strconv" + "strings" "testing" "github.com/alibaba/open-code-review/internal/llm" @@ -950,14 +952,115 @@ func TestSetConfigValueProviderClearsModel(t *testing.T) { } func TestRunConfigUnset_InvalidKey(t *testing.T) { - if err := runConfigUnset("provider"); err == nil { - t.Fatal("expected error for non custom_providers key") - } if err := runConfigUnset("custom_providers."); err == nil { t.Fatal("expected error for empty provider name") } } +func TestRunConfigSetWarnsWhenActiveProviderShadowsLegacyLLMConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + if err := saveConfig(configPath, &Config{Provider: "dashscope"}); err != nil { + t.Fatalf("save config: %v", err) + } + + stderr := captureConfigStderr(t, func() { + if err := runConfigSet("llm.url", "https://gateway.example/v1"); err != nil { + t.Fatalf("runConfigSet: %v", err) + } + }) + if !strings.Contains(stderr, `provider "dashscope" is active`) { + t.Errorf("warning = %q", stderr) + } + if !strings.Contains(stderr, "providers.dashscope.") || !strings.Contains(stderr, "config unset provider") { + t.Errorf("warning does not explain how to resolve precedence: %q", stderr) + } + + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload config: %v", err) + } + if cfg.Llm.URL != "https://gateway.example/v1" { + t.Errorf("llm.url = %q", cfg.Llm.URL) + } +} + +func TestLegacyLLMShadowWarning(t *testing.T) { + if got := legacyLLMShadowWarning("", "llm.model"); got != "" { + t.Errorf("warning without active provider = %q", got) + } + if got := legacyLLMShadowWarning("dashscope", "providers.dashscope.url"); got != "" { + t.Errorf("warning for provider setting = %q", got) + } + if got := legacyLLMShadowWarning("dashscope", "Llm.model"); got != "" { + t.Errorf("warning for invalid mixed-case legacy key = %q", got) + } + if got := legacyLLMShadowWarning("dashscope", "llm.model"); !strings.Contains(got, "providers.dashscope.") { + t.Errorf("preset-provider warning = %q", got) + } + if got := legacyLLMShadowWarning("my-gateway", "llm.model"); !strings.Contains(got, "custom_providers.my-gateway.") { + t.Errorf("custom-provider warning = %q", got) + } +} + +func TestRunConfigUnsetProviderClearsSelectionAndKeepsProviderEntries(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + if err := saveConfig(configPath, &Config{ + Provider: "dashscope", + Model: "legacy-model", + Providers: map[string]ProviderEntry{ + "dashscope": {APIKey: "secret", Model: "provider-model"}, + }, + }); err != nil { + t.Fatalf("save config: %v", err) + } + + if err := runConfigUnset("provider"); err != nil { + t.Fatalf("runConfigUnset(provider): %v", err) + } + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload config: %v", err) + } + if cfg.Provider != "" || cfg.Model != "" { + t.Errorf("provider/model = %q/%q, want both empty", cfg.Provider, cfg.Model) + } + if got := cfg.Providers["dashscope"].APIKey; got != "secret" { + t.Errorf("provider entry was removed or changed: api_key = %q", got) + } +} + +func captureConfigStderr(t *testing.T, fn func()) string { + t.Helper() + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stderr = w + defer func() { os.Stderr = old }() + + fn() + if err := w.Close(); err != nil { + t.Fatal(err) + } + data, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if err := r.Close(); err != nil { + t.Fatal(err) + } + return string(data) +} + func TestRunConfig_EmptyArgs(t *testing.T) { err := runConfig(nil) if err != nil { diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 18250ed7..67bb96dc 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -285,7 +285,7 @@ func parseConfigArgs(args []string) (configAction, error) { }, nil case "unset": if len(args) < 2 { - return configAction{}, fmt.Errorf("usage: ocr config unset custom_providers. | mcp_servers.\ne.g., ocr config unset custom_providers.my-gateway\ne.g., ocr config unset mcp_servers.codegraph") + return configAction{}, fmt.Errorf("usage: ocr config unset |mcp_servers.>\nexamples:\n ocr config unset provider\n ocr config unset custom_providers.my-provider\n ocr config unset mcp_servers.github") } return configAction{ subCmd: "unset", @@ -301,6 +301,7 @@ func printConfigUsage() { Usage: ocr config set + ocr config unset provider Disable provider-based configuration ocr config unset custom_providers. Delete a custom provider ocr config unset mcp_servers. Delete an MCP server ocr config provider Interactive provider setup @@ -329,6 +330,9 @@ Examples: # Delete a custom provider ocr config unset custom_providers.my-gateway + # Disable provider-based configuration and use legacy llm.* settings + ocr config unset provider + # MCP server configuration (stdio transport) ocr config set mcp_servers.codegraph.command npx ocr config set mcp_servers.codegraph.args '["-y","@anthropic/codegraph-mcp"]' diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 55a8b9df..f9ebe71a 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -1,6 +1,7 @@ package main import ( + "strings" "testing" "time" ) @@ -198,6 +199,11 @@ func TestParseConfigArgs_UnsetMissingKey(t *testing.T) { if err == nil { t.Fatal("expected error for missing key") } + for _, example := range []string{"ocr config unset provider", "ocr config unset custom_providers.my-provider", "ocr config unset mcp_servers.github"} { + if !strings.Contains(err.Error(), example) { + t.Errorf("error missing example %q: %v", example, err) + } + } } func TestParseConfigArgs_UnknownSubCmd(t *testing.T) { From d55f5e5940fcbe1881b31c7b7ff8bc921462c2c2 Mon Sep 17 00:00:00 2001 From: Abdul Moiz Hussain Date: Thu, 30 Jul 2026 14:27:13 +0500 Subject: [PATCH 4/7] refactor(diff): strip index headers from review prompts (#609) --- internal/diff/parser.go | 5 +++++ internal/diff/parser_test.go | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/internal/diff/parser.go b/internal/diff/parser.go index 9dd0fb4f..0cb09fcf 100644 --- a/internal/diff/parser.go +++ b/internal/diff/parser.go @@ -65,6 +65,11 @@ func ParseDiffText(ctx context.Context, diffText string, repoDir string, ref str switch { case strings.HasPrefix(line, "@@"): inHunk = true + // The object IDs and mode in Git's extended "index" header are not + // useful review context. Keep index text in hunks, where it is file + // content and therefore carries a diff prefix. + case !inHunk && strings.HasPrefix(line, "index "): + continue case !inHunk && binaryRe.MatchString(line): current.IsBinary = true // Extended header lines (unambiguous: content lines always carry a diff --git a/internal/diff/parser_test.go b/internal/diff/parser_test.go index b566bf58..78932707 100644 --- a/internal/diff/parser_test.go +++ b/internal/diff/parser_test.go @@ -2,9 +2,51 @@ package diff import ( "context" + "strings" "testing" ) +func TestParseDiffText_StripsIndexHeadersFromPromptDiff(t *testing.T) { + diffText := `diff --git a/first.go b/first.go +index 1234567..89abcde 100644 +--- a/first.go ++++ b/first.go +@@ -1,1 +1,2 @@ + first ++index added-content +diff --git a/second.go b/second.go +new file mode 100644 +index 0000000..7654321 +--- /dev/null ++++ b/second.go +@@ -0,0 +1 @@ ++package second +` + + diffs, err := ParseDiffText(context.Background(), diffText, t.TempDir(), "", nil) + if err != nil { + t.Fatalf("ParseDiffText: %v", err) + } + if len(diffs) != 2 { + t.Fatalf("expected 2 diffs, got %d", len(diffs)) + } + + for _, d := range diffs { + if strings.Contains(d.Diff, "\nindex ") { + t.Errorf("prompt diff contains index header:\n%s", d.Diff) + } + } + if !strings.Contains(diffs[0].Diff, "diff --git a/first.go b/first.go") { + t.Errorf("prompt diff lost git header:\n%s", diffs[0].Diff) + } + if !strings.Contains(diffs[0].Diff, "+index added-content") { + t.Errorf("prompt diff lost index-prefixed hunk content:\n%s", diffs[0].Diff) + } + if !diffs[1].IsNew { + t.Error("new-file metadata was not preserved") + } +} + // TestParseDiffText_Rename guards against issue #99: a renamed file must be // recognized via the "rename from"/"rename to" extended header lines so that // the parser reads content at the NEW path instead of warning about the old From d3ded74b57f3938e3c9b88f6f5f1bc39bcbe6730 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Fri, 17 Jul 2026 22:00:52 +0200 Subject: [PATCH 5/7] feat(config): resolve api_key/auth_token from a command (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `api_key_cmd` (provider entries) and `auth_token_cmd` (legacy llm block) so the LLM credential can be fetched from a secret manager at review time instead of stored plaintext in config.json — same pattern as git credential.helper / AWS credential_process. Resolution precedence (single site, presets and custom providers alike): static api_key always wins (stderr warning if a command is also set) → api_key_cmd → preset env var → error. The legacy llm block gets a mirrored auth_token_cmd; an incomplete legacy block never executes the command, and a set-but-failing command on a complete block is a hard error (never a silent fallback). Command execution is a build-tag split (sh -c / cmd /C) with a 60s timeout; the child's stderr passes through so pinentry/1Password/op prompts stay visible. Stdout is trimmed and used in memory only — never written to config or logged. Empty, whitespace-only, multi-line, and timed-out output are all hard errors. No caching (resolution runs once per process). - config set: api_key_cmd/auth_token_cmd are settable and round-trip; not masked (they are command lines, not secrets). - TUI cloneProviderEntry preserves api_key_cmd. - docs: 'API key from a command' section in configuration.md (en/zh/ja). Tests: table-driven runner matrix (success/trim/non-zero/empty/ whitespace/multi-line/not-found/timeout) + resolver precedence and legacy-fallthrough rows. Coverage 81.3%; Windows arm compile-checked (CI is Linux-only). --- cmd/opencodereview/config_cmd.go | 10 +- cmd/opencodereview/provider_tui.go | 1 + cmd/opencodereview/provider_tui_funcs_test.go | 4 + internal/llm/keycmd.go | 50 ++++++ internal/llm/keycmd_test.go | 72 +++++++++ internal/llm/keycmd_unix.go | 12 ++ internal/llm/keycmd_windows.go | 12 ++ internal/llm/resolver.go | 49 ++++-- internal/llm/resolver_keycmd_test.go | 143 ++++++++++++++++++ pages/src/content/docs/en/configuration.md | 20 +++ pages/src/content/docs/ja/configuration.md | 20 +++ pages/src/content/docs/zh/configuration.md | 16 ++ 12 files changed, 398 insertions(+), 11 deletions(-) create mode 100644 internal/llm/keycmd.go create mode 100644 internal/llm/keycmd_test.go create mode 100644 internal/llm/keycmd_unix.go create mode 100644 internal/llm/keycmd_windows.go create mode 100644 internal/llm/resolver_keycmd_test.go diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 3044c03f..1b2a3abb 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -226,6 +226,7 @@ func deleteCustomProvider(cfg *Config, name string) (bool, error) { // ProviderEntry holds per-provider configuration in the providers map. type ProviderEntry struct { APIKey string `json:"api_key,omitempty"` + APIKeyCmd string `json:"api_key_cmd,omitempty"` // shell command whose stdout is the api key; used when api_key is empty URL string `json:"url,omitempty"` Protocol string `json:"protocol,omitempty"` Model string `json:"model,omitempty"` @@ -264,6 +265,7 @@ type Config struct { type LlmConfig struct { URL string `json:"url,omitempty"` AuthToken string `json:"auth_token,omitempty"` + AuthTokenCmd string `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty AuthHeader string `json:"auth_header,omitempty"` Model string `json:"model,omitempty"` Protocol string `json:"protocol,omitempty"` // canonical protocol name; takes priority over UseAnthropic @@ -369,6 +371,8 @@ func setConfigValue(cfg *Config, key, value string) error { cfg.Llm.URL = value case "llm.auth_token", "llm.AuthToken": cfg.Llm.AuthToken = value + case "llm.auth_token_cmd", "llm.AuthTokenCmd": + cfg.Llm.AuthTokenCmd = value case "llm.auth_header", "llm.AuthHeader": normalized, err := llm.NormalizeAuthHeader(value) if err != nil { @@ -443,7 +447,7 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.ExtraBody = m default: - return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key) + return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key) } return nil } @@ -452,6 +456,8 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { switch field { case "api_key": entry.APIKey = value + case "api_key_cmd": + entry.APIKeyCmd = value case "url": entry.URL = value case "protocol": @@ -487,7 +493,7 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { } entry.ExtraHeaders = parsed default: - return fmt.Errorf("unknown provider field %q: supported fields are api_key, url, protocol, model, models, auth_header, extra_body, extra_headers", field) + return fmt.Errorf("unknown provider field %q: supported fields are api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers", field) } return nil } diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index b2ba1d17..7d2113f3 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -1181,6 +1181,7 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { func cloneProviderEntry(v ProviderEntry) ProviderEntry { out := ProviderEntry{ APIKey: v.APIKey, + APIKeyCmd: v.APIKeyCmd, URL: v.URL, Protocol: v.Protocol, Model: v.Model, diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 1462b3a7..b32c5663 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -198,6 +198,7 @@ func TestRenderListName_Inactive(t *testing.T) { func TestCloneProviderEntry_WithExtraBody(t *testing.T) { orig := ProviderEntry{ APIKey: "key", + APIKeyCmd: "op read op://dev/anthropic/api-key", URL: "http://localhost", Protocol: "openai", Model: "gpt-4", @@ -210,6 +211,9 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { if clone.APIKey != orig.APIKey || clone.URL != orig.URL || clone.Protocol != orig.Protocol { t.Error("basic fields not copied") } + if clone.APIKeyCmd != orig.APIKeyCmd { + t.Errorf("APIKeyCmd not copied: got %q, want %q", clone.APIKeyCmd, orig.APIKeyCmd) + } if len(clone.Models) != 2 || clone.Models[0] != "gpt-4" { t.Errorf("Models not cloned: %v", clone.Models) } diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go new file mode 100644 index 00000000..cd6ee2ba --- /dev/null +++ b/internal/llm/keycmd.go @@ -0,0 +1,50 @@ +package llm + +import ( + "context" + "fmt" + "os" + "strings" + "time" +) + +// keyCmdTimeout bounds how long an api_key_cmd / auth_token_cmd may run. +// It is a package var (not const) so tests can shrink it. +var keyCmdTimeout = 60 * time.Second + +// resolveKeyCmd runs a credential-fetching shell command and returns its +// trimmed, single-line stdout. label names the source (e.g. +// `api_key_cmd for provider "x"`) and is used in error messages. +// +// The child's stderr is wired to the process stderr so interactive prompts +// (pinentry, 1Password, `op`) stay visible. Any failure is a hard error, never +// a silent fallback. The resolved credential is used in memory only and is +// never written to config or logged. +func resolveKeyCmd(cmd, label string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout) + defer cancel() + + c := newKeyCmd(ctx, cmd) + c.Stderr = os.Stderr + + out, err := c.Output() + if ctx.Err() == context.DeadlineExceeded { + return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + } + if err != nil { + // Covers non-zero exit and command-not-found (the shell exits non-zero + // and prints its not-found message on the child's stderr). + return "", fmt.Errorf("%s failed: %w", label, err) + } + + // Trim a trailing line break; multi-line output past that is ambiguous and refused. + trimmed := strings.TrimRight(string(out), "\r\n") + if strings.Contains(trimmed, "\n") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + } + key := strings.TrimSpace(trimmed) + if key == "" { + return "", fmt.Errorf("%s produced empty output", label) + } + return key, nil +} diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go new file mode 100644 index 00000000..1ba66d42 --- /dev/null +++ b/internal/llm/keycmd_test.go @@ -0,0 +1,72 @@ +//go:build !windows + +package llm + +import ( + "strings" + "testing" + "time" +) + +func TestResolveKeyCmd(t *testing.T) { + tests := []struct { + name string + cmd string + want string + wantErr string // substring the error must contain; "" means success + }{ + {name: "success", cmd: "printf 'sk-test\\n'", want: "sk-test"}, + {name: "trailing whitespace trimmed", cmd: "printf ' sk-test \\n'", want: "sk-test"}, + {name: "no trailing newline", cmd: "printf 'sk-test'", want: "sk-test"}, + {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, + {name: "false", cmd: "false", wantErr: "failed:"}, + {name: "empty output", cmd: "true", wantErr: "produced empty output"}, + {name: "empty printf", cmd: "printf ''", wantErr: "produced empty output"}, + {name: "whitespace-only output", cmd: "printf ' \\n'", wantErr: "produced empty output"}, + {name: "multi-line output", cmd: "printf 'a\\nb\\n'", wantErr: "produced multi-line output"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (output %q)", tt.wantErr, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveKeyCmd_Timeout(t *testing.T) { + orig := keyCmdTimeout + keyCmdTimeout = 50 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout = orig }) + + _, err := resolveKeyCmd("sleep 5", "api_key_cmd for provider \"x\"") + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("false", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go new file mode 100644 index 00000000..81d3ebb3 --- /dev/null +++ b/internal/llm/keycmd_unix.go @@ -0,0 +1,12 @@ +//go:build !windows + +package llm + +import ( + "context" + "os/exec" +) + +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "sh", "-c", cmd) +} diff --git a/internal/llm/keycmd_windows.go b/internal/llm/keycmd_windows.go new file mode 100644 index 00000000..6253c330 --- /dev/null +++ b/internal/llm/keycmd_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package llm + +import ( + "context" + "os/exec" +) + +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "cmd", "/C", cmd) +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 853f42fd..6c7ffaa2 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -212,9 +212,10 @@ type llmFileConfig struct { AuthToken string `json:"auth_token,omitempty"` AuthHeader string `json:"auth_header,omitempty"` Model string `json:"model,omitempty"` - Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic - UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty - TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds + AuthTokenCmd string `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty + Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic + UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty + TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` } @@ -222,6 +223,7 @@ type llmFileConfig struct { // providerEntryConfig represents a single provider entry in config.json. type providerEntryConfig struct { APIKey string `json:"api_key,omitempty"` + APIKeyCmd string `json:"api_key_cmd,omitempty"` // shell command whose stdout is the api key; used when api_key is empty URL string `json:"url,omitempty"` Protocol string `json:"protocol,omitempty"` Model string `json:"model,omitempty"` @@ -282,13 +284,24 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, } apiKey := entry.APIKey - if apiKey == "" { - if isPreset && preset.EnvVar != "" { - apiKey = os.Getenv(preset.EnvVar) + switch { + case apiKey != "": + // Static api_key always wins. Warn (don't error) if a command is also set, + // so a config that keeps api_key_cmd as a deliberate fallback still works. + if entry.APIKeyCmd != "" { + fmt.Fprintf(os.Stderr, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case entry.APIKeyCmd != "": + resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err } + apiKey = resolved + case isPreset && preset.EnvVar != "": + apiKey = os.Getenv(preset.EnvVar) } if apiKey == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) } var url, protocol, authHeader, model string @@ -408,9 +421,27 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, if modelOverride != "" { model = modelOverride } - if cfg.Llm.URL == "" || cfg.Llm.AuthToken == "" || model == "" { + // Fall through to later strategies when the legacy block is incomplete. This + // includes the case where neither auth_token nor auth_token_cmd is set — and, + // critically, an incomplete block (e.g. missing url) never runs auth_token_cmd. + token := cfg.Llm.AuthToken + if cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { return ResolvedEndpoint{}, false, nil } + switch { + case token != "": + // Static auth_token always wins; warn if a command is also set. + if cfg.Llm.AuthTokenCmd != "" { + fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") + } + case cfg.Llm.AuthTokenCmd != "": + // Otherwise-complete legacy block with a set-but-failing command is a hard error. + resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } // llm.protocol (normalized) wins over use_anthropic when set. protocol := "" @@ -449,7 +480,7 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } - return ResolvedEndpoint{URL: cfg.Llm.URL, Token: cfg.Llm.AuthToken, Model: model, Protocol: protocol, AuthHeader: authHeader, Source: "OCR config file", ExtraBody: cfg.Llm.ExtraBody, ExtraHeaders: cfg.Llm.ExtraHeaders, Timeout: timeout}, true, nil + return ResolvedEndpoint{URL: cfg.Llm.URL, Token: token, Model: model, Protocol: protocol, AuthHeader: authHeader, Source: "OCR config file", ExtraBody: cfg.Llm.ExtraBody, ExtraHeaders: cfg.Llm.ExtraHeaders, Timeout: timeout}, true, nil } // tryCCEnv reads Claude Code environment variables. diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go new file mode 100644 index 00000000..f6b71582 --- /dev/null +++ b/internal/llm/resolver_keycmd_test.go @@ -0,0 +1,143 @@ +package llm + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeConfigJSON(t *testing.T, cfg configFile) string { + t.Helper() + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + p := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(p, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + return p +} + +// (a) api_key_cmd resolves when no static key is present. +func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q", ep.Token, "sk-from-cmd") + } +} + +// (b) static api_key wins even when api_key_cmd is also set. +func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } +} + +// (c) custom provider with api_key_cmd resolves (custom providers have no env fallback). +func TestResolveEndpoint_CustomProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + APIKeyCmd: "printf 'gw-token\\n'", + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-8b", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "gw-token" { + t.Errorf("Token = %q, want %q", ep.Token, "gw-token") + } +} + +// (d) a failing api_key_cmd is a hard error, not a silent fallback. +func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected hard error from failing api_key_cmd, got nil") + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } +} + +// (e) legacy auth_token_cmd resolves on an otherwise-complete llm block. +func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "printf 'legacy-token\\n'", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-token" { + t.Errorf("Token = %q, want %q", ep.Token, "legacy-token") + } +} + +// (f) an incomplete legacy block (missing url) with auth_token_cmd set does NOT +// run the command and falls through to later strategies. +func TestResolveEndpoint_LegacyIncompleteDoesNotRunCmd(t *testing.T) { + clearAllEnv(t) + // Command would exit non-zero if ever executed; if it ran, we'd see that + // error instead of the generic "no valid endpoint" fall-through error. + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + // URL intentionally omitted -> incomplete + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected no-endpoint error, got nil") + } + if strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("command should not have run for incomplete legacy config; error: %v", err) + } + if !strings.Contains(err.Error(), "no valid LLM endpoint") { + t.Errorf("expected fall-through no-endpoint error, got: %v", err) + } +} diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 1f3c19e0..2ffc2f70 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -124,6 +124,26 @@ The `timeout_sec` keys are not supported by `ocr config set` — edit } } ``` +### API key from a command + +Instead of storing a key in the config file, `api_key_cmd` fetches it at +runtime from a secret manager (1Password, `pass`, `gopass`, …). Its trimmed, +single-line stdout becomes the key. The same option is available for the +legacy `llm` block as `auth_token_cmd`. + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +Precedence: a static `api_key` always wins (if both are set, the command is +ignored and a warning is printed); otherwise `api_key_cmd` runs; only if +neither is set does OCR fall back to the provider's environment variable. + +The command runs once per `ocr` invocation and must succeed: a non-zero exit, +empty output, or multi-line output is a hard error (OCR never silently falls +back). It must complete within 60 seconds. The command's stderr is passed +through to your terminal, so interactive prompts (pinentry, Touch ID) still +work. ### Verify connectivity diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 64172888..0102f0db 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -122,6 +122,26 @@ Ollama は API key を無視しますが、カスタム provider は空でない } } ``` +### API key をコマンドで取得する + +key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット +マネージャー（1Password、`pass`、`gopass` など）から取得できます。前後の空白を +除いた 1 行の stdout が key になります。レガシーの `llm` ブロックにも同等の +`auth_token_cmd` があります。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +優先順位：静的な `api_key` が常に優先されます（両方設定されている場合はコマンドを +無視し、警告を表示します）。それ以外の場合は `api_key_cmd` を実行します。どちらも +設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 + +コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 +空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする +ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの +stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も +引き続き動作します。 ### 接続性を検証する diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index ffb0914c..5e59f9e8 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -116,6 +116,22 @@ provider 没有环境变量回退），所以设任意占位值即可。模型 } } ``` +### 通过命令获取 API key + +除了把 key 直接写进配置文件，还可以用 `api_key_cmd` 在运行时从密钥管理器 +（1Password、`pass`、`gopass` 等）获取。命令去除首尾空白后的单行 stdout 即为 +key。旧版 `llm` 配置块也有对应的 `auth_token_cmd`。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +优先级：静态 `api_key` 始终优先（两者都设置时忽略命令并打印警告）；否则运行 +`api_key_cmd`；只有两者都未设置时，OCR 才回退到 provider 对应的环境变量。 + +命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 +被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 +到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 ### 验证连通性 From fab5a3570d312b12d40fefa4209d3820df001dcd Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 09:49:47 +0200 Subject: [PATCH 6/7] fix(config): cloneProviderEntry drops timeout_sec and extra_headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cloneProviderEntry copied fields by hand and was never updated when TimeoutSec and ExtraHeaders were added to ProviderEntry, so editing any provider through `ocr config provider` silently erased both from the saved config: a per-provider timeout_sec of 900 reverted to the 300s default and custom extra_headers disappeared. Return the literal directly and use maps.Clone for both maps, which also preserves nil (matching each field's omitempty) where the old hand-rolled loop only special-cased ExtraBody. The regression test walks ProviderEntry with reflection and fails if the fixture leaves any field zero, so the next added field cannot be dropped without the DeepEqual catching it. Pre-existing bug, independent of the api_key_cmd work in this branch — separate commit so it can be bisected or cherry-picked on its own. --- cmd/opencodereview/provider_tui.go | 16 ++++------ cmd/opencodereview/provider_tui_funcs_test.go | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 7d2113f3..17cd6f7b 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "maps" "os" "sort" "strings" @@ -1179,7 +1180,7 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { // map cloning) can safely mutate the returned value without aliasing the // original's slice or map fields. func cloneProviderEntry(v ProviderEntry) ProviderEntry { - out := ProviderEntry{ + return ProviderEntry{ APIKey: v.APIKey, APIKeyCmd: v.APIKeyCmd, URL: v.URL, @@ -1187,15 +1188,12 @@ func cloneProviderEntry(v ProviderEntry) ProviderEntry { Model: v.Model, Models: append([]string(nil), v.Models...), AuthHeader: v.AuthHeader, + TimeoutSec: v.TimeoutSec, + // Shallow copy only: nested maps/slices inside a value are not cloned. + // maps.Clone keeps a nil map nil, matching the field's omitempty. + ExtraBody: maps.Clone(v.ExtraBody), + ExtraHeaders: maps.Clone(v.ExtraHeaders), } - if v.ExtraBody != nil { - out.ExtraBody = make(map[string]any, len(v.ExtraBody)) - for k, val := range v.ExtraBody { - // Shallow copy only: nested maps/slices inside val are not cloned. - out.ExtraBody[k] = val - } - } - return out } func cloneCustomProvidersMap(src map[string]ProviderEntry) map[string]ProviderEntry { diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index b32c5663..779d9863 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -3,6 +3,7 @@ package main import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -246,6 +247,37 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { } } +// TestCloneProviderEntry_CopiesEveryField fails when a field is added to +// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and +// ExtraHeaders were silently dropped. DeepEqual catches a dropped field; the +// reflect sweep is what stops a zero-valued fixture from hiding one. +func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { + orig := ProviderEntry{ + APIKey: "key", + APIKeyCmd: "op read op://dev/x/api-key", + URL: "http://localhost", + Protocol: "openai", + Model: "gpt-4", + Models: []string{"gpt-4"}, + AuthHeader: "Authorization", + TimeoutSec: 45, + ExtraBody: map[string]any{"temperature": 0.7}, + ExtraHeaders: map[string]string{"X-Trace": "on"}, + } + + rv := reflect.ValueOf(orig) + for i := range rv.NumField() { + if rv.Field(i).IsZero() { + t.Fatalf("fixture leaves %s zero-valued; set it so the clone is actually checked", + rv.Type().Field(i).Name) + } + } + + if clone := cloneProviderEntry(orig); !reflect.DeepEqual(clone, orig) { + t.Errorf("clone dropped a field:\n got %+v\nwant %+v", clone, orig) + } +} + func TestCustomListCount(t *testing.T) { cfg := &Config{ CustomProviders: map[string]ProviderEntry{ From fe68c63f8cfc89c0f2ea82664c281321fed6aca1 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 10:12:52 +0200 Subject: [PATCH 7/7] fix(llm): harden the credential command and cover it on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on the api_key_cmd/auth_token_cmd path, plus the CI job that actually exercises its Windows arm. The 60s timeout was not a real bound. It killed the shell, but a helper that leaves a background process holding the inherited stdout pipe (gpg-agent, pinentry, a first-use `op` daemon) kept Cmd.Wait blocked on the read long after the context died — `api_key_cmd = "sleep 200 & printf tok"` hung for over 90s. Buffer stdout through a writer os/exec copies in its own goroutine and set WaitDelay, which is what lets Wait force the pipe closed; ErrWaitDelay on its own is not a failure, since the command exited and its output is already buffered. Three more ways a resolved value could not be used: - Stdin was /dev/null, so a helper needing a passphrase saw EOF or refused to prompt for lack of a tty. Wired to os.Stdin, which is safe because no path resolves an endpoint while the bubbletea TUI is reading stdin. - Output was unbounded; `cat /dev/urandom` grew the heap without limit. Capped at 64KiB, refusing the write so the child dies of SIGPIPE. - Control bytes reached the Authorization header, where net/http rejects them as an opaque `invalid header field value`. Rejected up front with the offending byte and offset, matching httpguts.ValidHeaderFieldValue. A lone interior CR survived both TrimRight and TrimSpace, so it is now caught as multi-line output. Ordering: the command ran before the rest of the config was known to be usable, so `ocr review --model nonexistent` fired a biometric prompt and only then failed on the model name. Execution is deferred past validation at both sites — the source selection in tryProviderConfig, and ResolveEndpointWithModelOverride, which parsed OCR_LLM_TIMEOUT and OCR_LLM_EXTRA_HEADERS after resolving the credential. A whitespace-only static api_key also used to win precedence over a working api_key_cmd and send `Authorization: Bearer `; it now normalizes to unset, and the Manual TUI tab trims its token like the other two tabs. `ocr config provider` rejected api_key_cmd-only providers in both directions: non-interactively applyOfficialProviderConfig demanded a static key or an env var, and interactively the API-key step could not be confirmed because the field renders blank for such a provider. Both now treat a configured command as satisfying the requirement, and the error messages name the option that would fix it. Windows: the command line goes to cmd.exe through SysProcAttr.CmdLine with /S rather than through Args, because os/exec quotes Args with syscall.EscapeArg, which targets CommandLineToArgvW; cmd.exe is a documented exception whose escaping mangles any command containing a double quote, so `op read "op://Private/My Vault/api-key"` arrived as a single literal filename. Args stays at its one-element default rather than nil (syscall.StartProcess ignores argv when CmdLine is set) so Cmd.String() cannot panic on Args[1:]. CI ran only self-hosted Linux, and the cross-compile job proves the windows arms compile but never runs them, so keycmd_windows.go had zero coverage on any platform. Adds a windows-latest job that vets, tests, builds and smoke-tests natively. It installs Go with setup-go instead of the shared golang:1.26.5 image because GitHub does not support `container:` on Windows runners (actions/runner#904); no -race, since the detector needs a C toolchain there and races are OS-independent; no coverage gate, since the //go:build !windows files legitimately put the total under the Linux job's 80%. Six existing tests needed a guard for that job, none a behavior change: three assert an unreadable path is skipped, but Chmod(0000) on Windows only sets the read-only bit (and their os.Getuid() == 0 guard cannot cover it, since Getuid returns -1 there); TestSaveConfig asserts the 0600 the config is written with, which Windows reports as 0666; the symlink-safety test needs a privilege an unelevated CI account lacks; and the "absolute unchanged" background-path case was passing a rooted but non-absolute path, so it had been exercising the relative branch. Docs (en/zh/ja) spell out the failure modes, the 60s budget including the time spent answering a prompt, the inherited stdin/stderr, the extra 5s a daemon holding the pipe costs, and that config.json is trusted input because the value is executed as a shell command. Review follow-ups in the same pass. A whitespace-only api_key_cmd was the one credential field this path had not normalized: it is empty to `sh` but non-empty to Go, so it suppressed the env-var fallback and then failed with "produced empty output". It now reads as unset, the same as the equivalent typo in api_key. Same for auth_token_cmd on the legacy block. The TUI never showed that a command already satisfies the credential step, so the API-key field looked unconfigured on a provider that resolves fine; it now names the configured command on both the provider tabs and the Manual tab, rendering the command line and not the secret. The static-key-wins tests asserted only on the resolved token, which would have held just as well if the command ran and its output were discarded — i.e. a spurious biometric prompt on every review of a config that keeps a command as a fallback. They now use a filesystem witness to assert non-execution. The docs note that a command written for `sh` is generally not portable to `cmd.exe`, since the Windows arm is where that bites. --- .github/workflows/ci.yml | 49 +++ cmd/opencodereview/background_file_test.go | 8 + cmd/opencodereview/config_cmd.go | 12 +- cmd/opencodereview/config_cmd_test.go | 50 +++ cmd/opencodereview/flags.go | 4 +- cmd/opencodereview/flags_test.go | 53 +++ cmd/opencodereview/provider_cmd.go | 12 +- cmd/opencodereview/provider_cmd_test.go | 72 ++++- cmd/opencodereview/provider_tui.go | 83 ++++- cmd/opencodereview/provider_tui_funcs_test.go | 280 ++++++++++++---- cmd/opencodereview/provider_tui_test.go | 78 ++++- internal/config/rules/system_rules_test.go | 5 +- internal/llm/keycmd.go | 100 +++++- internal/llm/keycmd_test.go | 96 +++++- internal/llm/keycmd_unix.go | 2 + internal/llm/keycmd_windows.go | 37 ++- internal/llm/keycmd_windows_test.go | 152 +++++++++ internal/llm/resolver.go | 140 +++++--- internal/llm/resolver_keycmd_test.go | 302 +++++++++++++++++- internal/llm/resolver_test.go | 144 +++++++++ internal/viewer/handler_test.go | 7 + internal/viewer/store_load_test.go | 11 + pages/src/content/docs/en/configuration.md | 23 +- pages/src/content/docs/ja/configuration.md | 23 +- pages/src/content/docs/zh/configuration.md | 19 +- 25 files changed, 1603 insertions(+), 159 deletions(-) create mode 100644 internal/llm/keycmd_windows_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5bebad38..0782308c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,55 @@ jobs: echo "$HELP" | grep -q "rules" rm -f ./opencodereview + # Runs the suite natively on Windows, which the cross-compile job below cannot + # do: it only proves the windows arms of the build-tag splits compile. GitHub + # does not support `container:` on Windows runners + # (actions/runner#904), so this job installs Go directly instead of reusing the + # golang:1.26.5 image the other jobs share. + windows: + runs-on: windows-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-go@v7 + with: + go-version: '1.26.5' + cache: true + + - name: Vet + run: go vet ./... + + # No -race here: the race detector needs a working C toolchain on Windows, + # and races are OS-independent, so the Linux job above already covers them. + # This job is here for the OS-specific behavior instead. No coverage gate + # either -- the //go:build !windows test files legitimately drop the total + # below the 80% the Linux job enforces. + - name: Test + run: go test -count=1 ./... + + - name: Build + run: go build -o opencodereview.exe ./cmd/opencodereview + + # Same assertions as the Linux smoke test, under git-bash so the script is + # shared verbatim rather than reimplemented in PowerShell. + - name: Smoke test + shell: bash + run: | + ./opencodereview.exe --version + ./opencodereview.exe --version | grep -q "open-code-review" + HELP=$(./opencodereview.exe --help) + echo "$HELP" | grep -q "Commands:" + echo "$HELP" | grep -q "review" + echo "$HELP" | grep -q "scan" + echo "$HELP" | grep -q "delegate" + echo "$HELP" | grep -q "config" + echo "$HELP" | grep -q "llm" + echo "$HELP" | grep -q "viewer" + echo "$HELP" | grep -q "session" + echo "$HELP" | grep -q "rules" + rm -f ./opencodereview.exe + cross-compile: runs-on: self-hosted timeout-minutes: 10 diff --git a/cmd/opencodereview/background_file_test.go b/cmd/opencodereview/background_file_test.go index 827e52d7..39de56a8 100644 --- a/cmd/opencodereview/background_file_test.go +++ b/cmd/opencodereview/background_file_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" ) @@ -37,7 +38,14 @@ func TestResolveBackgroundFilePath(t *testing.T) { }) t.Run("absolute unchanged", func(t *testing.T) { + // FromSlash is not enough on its own: it only swaps separators, and + // `\etc\context.md` is rooted but not absolute on Windows, where + // filepath.IsAbs wants a volume. Without the drive letter this case + // exercised the relative branch instead of the one it names. abs := filepath.FromSlash("/etc/context.md") + if runtime.GOOS == "windows" { + abs = `C:\etc\context.md` + } if got := resolveBackgroundFilePath(repo, abs); got != abs { t.Errorf("resolveBackgroundFilePath = %q, want %q (absolute must be untouched)", got, abs) } diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 1b2a3abb..92f6dc56 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -85,8 +85,7 @@ func runConfigSet(key, value string) error { } displayValue := value - normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", "")) - if strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken") { + if shouldMaskConfigValue(key) { displayValue = maskKey(value) } fmt.Printf("Set %s = %s\n", key, displayValue) @@ -96,6 +95,15 @@ func runConfigSet(key, value string) error { return nil } +// shouldMaskConfigValue reports whether the echoed value of a config key holds a +// secret and must be masked. Matching on the normalized suffix covers both +// snake_case and Go field spellings of api_key/auth_token at any path depth, +// while the *_cmd variants stay unmasked: a command line is not a secret. +func shouldMaskConfigValue(key string) bool { + normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", "")) + return strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken") +} + func runConfigUnset(key string) error { configPath, err := defaultConfigPath() if err != nil { diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index f2b4b246..e7e88650 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -89,6 +89,56 @@ func TestSetConfigValueProviderEntry(t *testing.T) { } } +func TestSetConfigValueKeyCmdFields(t *testing.T) { + // A typo in any of these case labels would silently degrade to "unknown + // provider field" / "unknown config key", so assert the field each key writes. + const value = "op read op://dev/anthropic/api-key" + tests := []struct { + name string + key string + got func(cfg *Config) string + }{ + {"preset provider api_key_cmd", "providers.anthropic.api_key_cmd", func(cfg *Config) string { return cfg.Providers["anthropic"].APIKeyCmd }}, + {"custom provider api_key_cmd", "custom_providers.my-gateway.api_key_cmd", func(cfg *Config) string { return cfg.CustomProviders["my-gateway"].APIKeyCmd }}, + {"llm auth_token_cmd", "llm.auth_token_cmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }}, + {"llm AuthTokenCmd alias", "llm.AuthTokenCmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &Config{} + if err := setConfigValue(cfg, tt.key, value); err != nil { + t.Fatalf("setConfigValue %s: %v", tt.key, err) + } + if got := tt.got(cfg); got != value { + t.Errorf("%s = %q, want %q", tt.key, got, value) + } + }) + } +} + +func TestShouldMaskConfigValue(t *testing.T) { + // api_key/auth_token values are secrets; the *_cmd variants are command + // lines, so they print unmasked. + tests := []struct { + key string + want bool + }{ + {"llm.auth_token", true}, + {"llm.auth_token_cmd", false}, + {"providers.x.api_key", true}, + {"providers.x.api_key_cmd", false}, + {"providers.x.APIKeyCmd", false}, + {"llm.AuthToken", true}, + } + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + if got := shouldMaskConfigValue(tt.key); got != tt.want { + t.Errorf("shouldMaskConfigValue(%q) = %v, want %v", tt.key, got, tt.want) + } + }) + } +} + func TestSetConfigValueProviderEntryNonPresetWritesCustomProvider(t *testing.T) { cfg := &Config{} diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 67bb96dc..753a84ce 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -355,8 +355,8 @@ Examples: ocr config set language English ocr config set telemetry.enabled true -Supported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging -Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers +Supported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging +Provider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers Protocol values: anthropic, openai, openai-responses MCP server fields: type, command, args, env, url, headers, tools, setup`) } diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index f9ebe71a..f036e714 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -1,6 +1,7 @@ package main import ( + "slices" "strings" "testing" "time" @@ -232,6 +233,58 @@ func TestPrintDefaults(t *testing.T) { fs.PrintDefaults() } +// configFieldList returns the comma-separated names that follow prefix on the +// one line of text starting with it. +func configFieldList(t *testing.T, text, prefix string) []string { + t.Helper() + for _, line := range strings.Split(text, "\n") { + if !strings.HasPrefix(line, prefix) { + continue + } + var out []string + for _, field := range strings.Split(strings.TrimPrefix(line, prefix), ",") { + if field = strings.TrimSpace(field); field != "" { + out = append(out, field) + } + } + return out + } + t.Fatalf("no line starting with %q in:\n%s", prefix, text) + return nil +} + +// These four lists are duplicated verbatim in printConfigUsage (what `ocr config` +// and `ocr config --help` print) and in setConfigValue's unknown-key error. +// api_key_cmd and llm.auth_token_cmd were added to the second copy and missed in +// the first, so the primary discovery surface silently disagreed with the code. +// Compared in order, since both copies are meant to be identical text. +func TestPrintConfigUsage_ListsMatchSetConfigValueError(t *testing.T) { + usage := captureStdout(t, printConfigUsage) + + err := setConfigValue(&Config{}, "definitely.not.a.key", "") + if err == nil { + t.Fatal("setConfigValue should reject an unknown key") + } + canonical := err.Error() + + prefixes := []string{ + "Supported keys: ", + "Provider fields: ", + "Protocol values: ", + "MCP server fields: ", + } + for _, prefix := range prefixes { + t.Run(strings.TrimSuffix(prefix, ": "), func(t *testing.T) { + want := configFieldList(t, canonical, prefix) + got := configFieldList(t, usage, prefix) + if !slices.Equal(got, want) { + t.Errorf("%q drifted between flags.go and config_cmd.go\n flags.go: %v\n config_cmd.go: %v", + prefix, got, want) + } + }) + } +} + func TestExpandShortFlags(t *testing.T) { m := map[string]string{"c": "commit", "f": "format"} tests := []struct { diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index f67da930..2a83abb3 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -235,13 +235,16 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider preset, isPreset := llm.LookupProvider(result.provider) - if result.apiKey == "" { + // Mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): + // an already-configured api_key_cmd satisfies the requirement, so picking a + // model for such a provider must not fail and abandon the save. + if result.apiKey == "" && cfg.Providers[result.provider].APIKeyCmd == "" { if isPreset && preset.EnvVar != "" { if os.Getenv(preset.EnvVar) == "" { - return fmt.Errorf("API key is required for provider %s (configure it or set $%s)", result.provider, preset.EnvVar) + return fmt.Errorf("API key is required for provider %s (configure it, set providers.%s.api_key_cmd, or set $%s)", result.provider, result.provider, preset.EnvVar) } } else { - return fmt.Errorf("API key is required for provider %s", result.provider) + return fmt.Errorf("API key is required for provider %s (configure it or set providers.%s.api_key_cmd)", result.provider, result.provider) } } @@ -257,7 +260,8 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider if result.apiKey != "" { entry.APIKey = result.apiKey } else { - // Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR. + // Confirmed empty key: clear saved api_key so the resolver falls back to + // api_key_cmd (when set) or $ENV_VAR. entry.APIKey = "" } cfg.Providers[result.provider] = entry diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index d1f03bba..a14a7d82 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -5,9 +5,37 @@ import ( "io" "os" "path/filepath" + "runtime" "testing" ) +// isolateLLMConnectionTest keeps the "Testing connection..." step that ends +// every apply*Config call away from the developer's own machine. Without it +// resolveConfigPath() falls back to ~/.opencodereview/config.json and `go test` +// resolves a real endpoint: with providers..api_key_cmd configured that +// runs the credential helper and blocks on a pinentry/Touch ID prompt for up to +// the 60s credential timeout, and with a static key it fires a real request. +// +// The path points at a file that does not exist, so resolution fails fast the +// way it already does on a machine with no config. HOME is redirected into an +// empty temp dir as well, so the shell-rc strategy has nothing to read either. +func isolateLLMConnectionTest(t *testing.T) { + t.Helper() + dir := t.TempDir() + t.Setenv("OCR_CONFIG_PATH", filepath.Join(dir, "no-such-config.json")) + // Both, because os.UserHomeDir reads USERPROFILE on Windows and never falls + // back to HOME -- setting HOME alone would leave the shell-rc strategy reading + // the real profile. + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + for _, k := range []string{ + "OCR_LLM_URL", "OCR_LLM_TOKEN", "OCR_LLM_MODEL", + "ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_MODEL", + } { + t.Setenv(k, "") + } +} + func TestMaskKey(t *testing.T) { tests := []struct { name string @@ -47,8 +75,12 @@ func TestSaveConfig(t *testing.T) { if err != nil { t.Fatalf("stat: %v", err) } - if perm := info.Mode().Perm(); perm != 0o600 { - t.Errorf("perm = %o, want 600", perm) + // Windows reports 0666 regardless of the mode passed to OpenFile, so only the + // unix arms can assert the 0600 the config file is written with. + if runtime.GOOS != "windows" { + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("perm = %o, want 600", perm) + } } data, err := os.ReadFile(path) @@ -206,6 +238,7 @@ func TestApplyOfficialProviderConfig_MissingFields(t *testing.T) { } func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { + isolateLLMConnectionTest(t) t.Setenv("DEEPSEEK_API_KEY", "sk-from-env") dir := t.TempDir() configPath := filepath.Join(dir, "config.json") @@ -240,7 +273,41 @@ func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { } } +// A provider configured with only api_key_cmd must survive a trip through the +// TUI: picking a model returns an empty apiKey, which must not be mistaken for +// "no credential" and abandon the save. +func TestApplyOfficialProviderConfig_APIKeyCmdSatisfiesRequirement(t *testing.T) { + isolateLLMConnectionTest(t) + t.Setenv("DEEPSEEK_API_KEY", "") + configPath := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + Providers: map[string]ProviderEntry{ + "deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"}, + }, + } + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "deepseek", + model: "deepseek-v4-flash", + apiKey: "", + }) + if err != nil { + t.Fatalf("api_key_cmd should satisfy the API key requirement: %v", err) + } + diskCfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if diskCfg.Provider != "deepseek" || diskCfg.Model != "deepseek-v4-flash" { + t.Errorf("save was abandoned: provider=%q model=%q", diskCfg.Provider, diskCfg.Model) + } + if got := diskCfg.Providers["deepseek"].APIKeyCmd; got != "op read op://dev/deepseek/api-key" { + t.Errorf("persisted api_key_cmd = %q, want it preserved", got) + } +} + func TestApplyCustomProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{ @@ -300,6 +367,7 @@ func TestProviderTUIResult_ResolvedModel(t *testing.T) { } func TestApplyOfficialProviderConfig_UsesSessionModelPick(t *testing.T) { + isolateLLMConnectionTest(t) t.Setenv("QIANFAN_API_KEY", "sk-from-env") dir := t.TempDir() configPath := filepath.Join(dir, "config.json") diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 17cd6f7b..bce95fa4 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -905,11 +905,36 @@ func officialProviderEnvKeySet(p llm.Provider) bool { return p.EnvVar != "" && os.Getenv(p.EnvVar) != "" } +// officialAPIKeyRequiredError mirrors the wording applyOfficialProviderConfig +// uses for the same failure, so the interactive and non-interactive paths name +// the same options in the same order (static key -> api_key_cmd -> env var). func officialAPIKeyRequiredError(p llm.Provider) string { + if p.Name == "" { + return "API key is required" + } if p.EnvVar != "" { - return fmt.Sprintf("API key is required (or set $%s)", p.EnvVar) + return fmt.Sprintf("API key is required (configure it, set providers.%s.api_key_cmd, or set $%s)", p.Name, p.EnvVar) + } + return fmt.Sprintf("API key is required (configure it or set providers.%s.api_key_cmd)", p.Name) +} + +// apiKeyCmdForStep returns the api_key_cmd already configured for the provider +// the API-key step is editing, reading the same config entry loadExistingAPIKey +// reads the static key from. The step serves the Official and Custom tabs; the +// Manual tab has its own form and uses llm.auth_token_cmd instead. +func (m providerTUIModel) apiKeyCmdForStep() string { + switch m.activeTab { + case tabOfficial: + if m.existingCfg == nil { + return "" + } + return m.existingCfg.Providers[m.currentProvider().Name].APIKeyCmd + case tabCustom: + if cp, ok := m.selectedCustomProvider(); ok { + return m.customProviderEntry(cp.name, cp.entry).APIKeyCmd + } } - return "API key is required" + return "" } func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { @@ -919,6 +944,12 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { if !m.apiKeyMasked && strings.TrimSpace(m.apiKeyInput.Value()) != "" { return true, "" } + // Resolver precedence is static key -> api_key_cmd -> env var, so an already + // configured command satisfies the requirement: the field renders blank for + // such a provider and must still be confirmable. + if m.apiKeyCmdForStep() != "" { + return true, "" + } if m.activeTab == tabOfficial { p := m.currentProvider() if officialProviderEnvKeySet(p) { @@ -926,6 +957,9 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { } return false, officialAPIKeyRequiredError(p) } + if cp, ok := m.selectedCustomProvider(); ok && cp.name != "" { + return false, fmt.Sprintf("API key is required (configure it or set custom_providers.%s.api_key_cmd)", cp.name) + } return false, "API key is required" } @@ -1044,7 +1078,16 @@ func authHeaderFormError(raw string) string { ) } -const manualAuthTokenRequiredError = "Auth token is required (whitespace-only input is not accepted)" +const manualAuthTokenRequiredError = "Auth token is required (configure it or set llm.auth_token_cmd; whitespace-only input is not accepted)" + +// manualAuthTokenCmd returns the configured llm.auth_token_cmd, which the +// resolver runs when llm.auth_token is empty. +func (m providerTUIModel) manualAuthTokenCmd() string { + if m.existingCfg == nil { + return "" + } + return m.existingCfg.Llm.AuthTokenCmd +} func (m providerTUIModel) handleCustomFormEnter() (tea.Model, tea.Cmd) { switch m.cpStep { @@ -1605,7 +1648,9 @@ func (m providerTUIModel) handleManualFormEnter() (tea.Model, tea.Cmd) { m.manualStep = manualStepAuthToken return m, m.manualTokenInput.Focus() case manualStepAuthToken: - if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" { + // Same precedence as the provider tabs: an already configured + // llm.auth_token_cmd stands in for a typed or saved token. + if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" && m.manualAuthTokenCmd() == "" { m.formError = manualAuthTokenRequiredError return m, nil } @@ -1907,7 +1952,10 @@ func (m providerTUIModel) result() providerTUIResult { return providerTUIResult{} case tabManual: - apiKey := m.manualTokenInput.Value() + // Trim like the Official and Custom tabs: a whitespace-only token must + // never persist, or it wins precedence over a working auth_token_cmd + // and sends "Authorization: Bearer ". + apiKey := strings.TrimSpace(m.manualTokenInput.Value()) if m.manualTokenMasked || (apiKey == "" && m.manualTokenOriginal != "") { apiKey = m.manualTokenOriginal } @@ -2212,6 +2260,9 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { if m.manualTokenMasked && m.manualTokenOriginal != "" { s.WriteString(tuiDimStyle.Render(" "+savedSecretReplaceHint(m.manualTokenOriginal)) + "\n") } + if cmd := m.manualAuthTokenCmd(); cmd != "" { + s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "llm.auth_token_cmd", cmd)) + "\n") + } case manualStepAuthHeader: s.WriteString(" " + m.manualAuthHeaderInput.View() + "\n") } @@ -2314,6 +2365,14 @@ func (m providerTUIModel) viewAPIKey(s *strings.Builder) { s.WriteString("\n") } + // Mirrors the env-var hint below: the step is already satisfied, so say so + // rather than leaving an empty field that looks unconfigured. + if cmd := m.apiKeyCmdForStep(); cmd != "" { + s.WriteString("\n") + s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "api_key_cmd", cmd))) + s.WriteString("\n") + } + if m.activeTab == tabOfficial { provider := m.currentProvider() if envKey := os.Getenv(provider.EnvVar); envKey != "" { @@ -2392,6 +2451,20 @@ func officialAPIKeyEnvSetHintLine(envVar string, hasSavedKey bool) string { return " " + officialAPIKeyEnvSetHint(envVar, hasSavedKey) } +// keyCmdConfiguredHint explains why this step accepts an empty field. A +// provider configured only by command renders a blank input -- the command line +// is not the secret, but it is also not the value being edited here -- so +// without this the user has no way to tell a credential is already wired up, +// and no way to know that leaving the field empty is the correct action. +// keyLabel names the config key so the hint points at what to edit instead. +func keyCmdConfiguredHint(keyLabel, cmd string) string { + return fmt.Sprintf("%s is set (%s); leave empty to keep using it.", keyLabel, cmd) +} + +func keyCmdConfiguredHintLine(indent, keyLabel, cmd string) string { + return indent + keyCmdConfiguredHint(keyLabel, cmd) +} + // --- Styles --- const tuiCursor = "▸" diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 779d9863..61765044 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -205,7 +205,11 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { Model: "gpt-4", Models: []string{"gpt-4", "gpt-3.5"}, AuthHeader: "Authorization", + TimeoutSec: 45, ExtraBody: map[string]any{"temperature": 0.7, "stream": true}, + ExtraHeaders: map[string]string{ + "X-Trace": "on", + }, } clone := cloneProviderEntry(orig) @@ -234,6 +238,22 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { if len(orig.Models) != 2 { t.Error("modifying clone should not affect original Models") } + + if clone.TimeoutSec != orig.TimeoutSec { + t.Errorf("TimeoutSec not copied: got %d, want %d", clone.TimeoutSec, orig.TimeoutSec) + } + if clone.ExtraHeaders == nil { + // Fatal, not Error: writing to the nil map below would panic instead of + // reporting which field was dropped. + t.Fatal("ExtraHeaders should not be nil") + } + if clone.ExtraHeaders["X-Trace"] != "on" { + t.Errorf("ExtraHeaders not copied: %v", clone.ExtraHeaders) + } + clone.ExtraHeaders["X-New"] = "1" + if _, ok := orig.ExtraHeaders["X-New"]; ok { + t.Error("modifying clone should not affect original ExtraHeaders") + } } func TestCloneProviderEntry_NilExtraBody(t *testing.T) { @@ -245,12 +265,17 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { if clone.ExtraBody != nil { t.Error("ExtraBody should remain nil") } + if clone.ExtraHeaders != nil { + t.Error("ExtraHeaders should remain nil") + } } -// TestCloneProviderEntry_CopiesEveryField fails when a field is added to -// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and -// ExtraHeaders were silently dropped. DeepEqual catches a dropped field; the -// reflect sweep is what stops a zero-valued fixture from hiding one. +// cloneProviderEntry lists fields by hand, which is how timeout_sec and +// extra_headers came to be silently dropped on the save-rollback paths. This +// fails when a field is added to ProviderEntry but not to the clone: the +// non-zero check forces the fixture to grow, and DeepEqual then catches the +// omission. It catches a dropped field, not an aliased one -- DeepEqual +// compares values, not identity; the sibling test above covers aliasing. func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { orig := ProviderEntry{ APIKey: "key", @@ -1812,83 +1837,198 @@ func TestProviderTUI_ResultUsesSessionModelPickWhenSelectionEmpty(t *testing.T) } } -func TestApiKeyStepCanConfirm_OfficialEmptyWithoutEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, +// apiKeyStepCanConfirm gates the final Enter of `ocr config provider`. It has to +// mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): +// a provider configured with only api_key_cmd renders a blank key field, and +// blocking it there made the feature unreachable from the documented wizard. +func TestApiKeyStepCanConfirm(t *testing.T) { + tests := []struct { + name string + env string + cfg *Config + customTab bool + typedKey string + wantOK bool + wantErrMsg string + }{ + { + name: "official saved api_key", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKey: "keep-me"}}, + }, + wantOK: true, }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required (or set $DEEPSEEK_API_KEY)" { - t.Errorf("errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_OfficialEmptyWithEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "sk-from-env") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, + { + name: "official typed key", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + typedKey: "sk-typed", + wantOK: true, }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_CustomEmpty(t *testing.T) { - cfg := &Config{ - Provider: "stepfun", - CustomProviders: map[string]ProviderEntry{ - "stepfun": {APIKey: ""}, + { + name: "official api_key_cmd only", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"}}, + }, + wantOK: true, + }, + { + name: "official nothing configured", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it, set providers.deepseek.api_key_cmd, or set $DEEPSEEK_API_KEY)", + }, + { + name: "official env var set", + env: "sk-from-env", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: true, + }, + { + name: "custom saved api_key", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKey: "sk-custom"}}, + }, + wantOK: true, + }, + { + name: "custom api_key_cmd only", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKeyCmd: "op read op://dev/stepfun/api-key"}}, + }, + wantOK: true, + }, + { + name: "custom nothing configured", + customTab: true, + cfg: &Config{Provider: "stepfun", CustomProviders: map[string]ProviderEntry{"stepfun": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it or set custom_providers.stepfun.api_key_cmd)", }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabCustom - m.customIdx = 0 - m.step = stepAPIKey - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required" { - t.Errorf("errMsg = %q", errMsg) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("DEEPSEEK_API_KEY", tc.env) + m := newProviderTUI(tc.cfg, "") + if tc.customTab { + m.activeTab = tabCustom + m.customIdx = 0 + } else { + m.activeTab = tabOfficial + } + m.step = stepAPIKey + // loadExistingAPIKey is what the wizard runs on entering the step, and + // is the only thing that populates apiKeyOriginal / the mask. + m.loadExistingAPIKey() + if tc.typedKey != "" { + m.apiKeyInput.SetValue(tc.typedKey) + } + + ok, errMsg := m.apiKeyStepCanConfirm() + if ok != tc.wantOK { + t.Fatalf("apiKeyStepCanConfirm() ok = %v, want %v (errMsg = %q)", ok, tc.wantOK, errMsg) + } + if errMsg != tc.wantErrMsg { + t.Errorf("errMsg = %q, want %q", errMsg, tc.wantErrMsg) + } + }) } } -func TestApiKeyStepCanConfirm_MaskedSavedKey(t *testing.T) { - cfg := &Config{ - Provider: "deepseek", - Providers: map[string]ProviderEntry{ - "deepseek": {APIKey: "keep-me"}, +// The Manual tab's auth-token gate is the legacy twin of apiKeyStepCanConfirm: +// llm.auth_token_cmd has to stand in for an empty field the same way. +func TestHandleManualFormEnter_AuthTokenGate(t *testing.T) { + tests := []struct { + name string + llmCfg LlmConfig + typedToken string + wantAdvance bool + // wantAPIKey is the token result() must persist once the step confirms. + wantAPIKey string + }{ + { + name: "saved auth_token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthToken: "tok-saved"}, + wantAdvance: true, + wantAPIKey: "tok-saved", + }, + { + name: "typed token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: "tok-typed", + wantAdvance: true, + wantAPIKey: "tok-typed", + }, + { + name: "auth_token_cmd only", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + wantAdvance: true, + }, + { + // auth_token_cmd opens the gate, so whitespace typed at this step + // confirms. It must not be saved as auth_token: a non-empty token + // wins precedence and would silently shadow the working command. + name: "auth_token_cmd with whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + typedToken: " ", + wantAdvance: true, + }, + { + name: "nothing configured", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + wantAdvance: false, + }, + { + name: "whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: " ", + wantAdvance: false, }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - m.loadExistingAPIKey() - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := newProviderTUI(&Config{Llm: tc.llmCfg}, "") + m.activeTab = tabManual + m.inManualForm = true + m.manualStep = manualStepAuthToken + if tc.typedToken != "" { + m.manualTokenInput.SetValue(tc.typedToken) + } + + result, _ := m.handleManualFormEnter() + m2 := result.(providerTUIModel) + + if tc.wantAdvance { + if m2.manualStep != manualStepAuthHeader { + t.Fatalf("manualStep = %d, want manualStepAuthHeader (%d); formError = %q", + m2.manualStep, manualStepAuthHeader, m2.formError) + } + if m2.formError != "" { + t.Errorf("formError = %q, want empty", m2.formError) + } + if got := m2.result().apiKey; got != tc.wantAPIKey { + t.Errorf("result().apiKey = %q, want %q", got, tc.wantAPIKey) + } + return + } + if m2.manualStep != manualStepAuthToken { + t.Fatalf("manualStep = %d, want to stay on manualStepAuthToken (%d)", + m2.manualStep, manualStepAuthToken) + } + if m2.formError != manualAuthTokenRequiredError { + t.Errorf("formError = %q, want %q", m2.formError, manualAuthTokenRequiredError) + } + if !strings.Contains(m2.formError, "llm.auth_token_cmd") { + t.Errorf("formError should name llm.auth_token_cmd, got %q", m2.formError) + } + }) } } diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index 69d41821..c20cd16b 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -2291,8 +2291,10 @@ func TestProviderTUI_OfficialApiKeyEmptyWithoutEnvBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required (or set $DASHSCOPE_API_KEY)" { - t.Errorf("formError = %q", m2.formError) + // The exact prose is pinned by TestApiKeyStepCanConfirm; this test covers the + // Enter-key wiring, so compare against the helper and never drift again. + if want := officialAPIKeyRequiredError(m2.currentProvider()); m2.formError != want { + t.Errorf("formError = %q, want %q", m2.formError, want) } if cmd != nil { t.Error("Enter without key or env should not quit") @@ -2354,8 +2356,10 @@ func TestProviderTUI_CustomExistingApiKeyEmptyBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required" { - t.Errorf("formError = %q, want %q", m2.formError, "API key is required") + // Prefix, not the full string: this test covers Enter-key gating, and the + // exact wording is pinned by TestApiKeyStepCanConfirm. + if !strings.HasPrefix(m2.formError, "API key is required") { + t.Errorf("formError = %q, want it to start with %q", m2.formError, "API key is required") } if cmd != nil { t.Error("Enter with cleared key should not quit") @@ -2600,6 +2604,7 @@ func TestProviderTUI_DeleteModelPreservesActiveModel(t *testing.T) { } func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") models := []string{"test-model", "test-model-2", "bbb", "aaa", "test-model-3"} @@ -2643,6 +2648,7 @@ func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { } func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{} @@ -2668,6 +2674,7 @@ func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { } func TestApplyCustomProviderConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{ @@ -2816,6 +2823,7 @@ func TestEnterEditCustomProvider_ProtocolIndex(t *testing.T) { // mirrored for the two protocols that have a boolean equivalent so older // binaries can still read the config. func TestApplyManualConfig_DoubleWritesProtocolAndUseAnthropic(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") @@ -2920,3 +2928,65 @@ func TestProviderTUIResult_ManualProtocolIsCanonical(t *testing.T) { } } } + +func TestKeyCmdConfiguredHint(t *testing.T) { + got := keyCmdConfiguredHint("api_key_cmd", "op read op://dev/anthropic/api-key") + want := "api_key_cmd is set (op read op://dev/anthropic/api-key); leave empty to keep using it." + if got != want { + t.Errorf("hint = %q, want %q", got, want) + } +} + +// A provider configured only by command renders a blank API-key field, so +// without this hint there is nothing on screen distinguishing "credential +// already wired up" from "nothing configured". +func TestProviderTUI_ViewAPIKey_ShowsAPIKeyCmdHint(t *testing.T) { + cfg := &Config{ + Provider: "deepseek", + Model: "deepseek-v4-flash", + Providers: map[string]ProviderEntry{ + "deepseek": {APIKeyCmd: "op read op://dev/deepseek/key", Model: "deepseek-v4-flash"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "deepseek" { + m.officialIdx = i + break + } + } + m.step = stepAPIKey + m.loadExistingAPIKey() + m.apiKeyInput.Focus() + + got := stripANSI(m.View().Content) + want := "api_key_cmd is set (op read op://dev/deepseek/key); leave empty to keep using it." + if !strings.Contains(got, want) { + t.Errorf("view missing api_key_cmd hint; want %q; got:\n%s", want, got) + } +} + +func TestProviderTUI_ViewAPIKey_NoCmdHintWhenUnset(t *testing.T) { + cfg := &Config{ + Provider: "deepseek", + Model: "deepseek-v4-flash", + Providers: map[string]ProviderEntry{ + "deepseek": {Model: "deepseek-v4-flash"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "deepseek" { + m.officialIdx = i + break + } + } + m.step = stepAPIKey + m.loadExistingAPIKey() + + if got := stripANSI(m.View().Content); strings.Contains(got, "api_key_cmd is set") { + t.Errorf("view should not claim api_key_cmd is set when it is not; got:\n%s", got) + } +} diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 10aca01b..dd978162 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1237,7 +1237,10 @@ func TestResolveRuleEntries_SymlinkSafety(t *testing.T) { // The extension check on the resolved path should reject .json. symlinkPath := filepath.Join(dir, "evil.md") if err := os.Symlink(sensitiveFile, symlinkPath); err != nil { - t.Fatal(err) + // Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which + // an unelevated CI account does not have. Same skip the other symlink tests + // in this repo already use. + t.Skipf("cannot create symlink: %v", err) } entries := []ProjectRuleEntry{ diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go index cd6ee2ba..0060c88e 100644 --- a/internal/llm/keycmd.go +++ b/internal/llm/keycmd.go @@ -1,9 +1,12 @@ package llm import ( + "bytes" "context" + "errors" "fmt" "os" + "os/exec" "strings" "time" ) @@ -12,36 +15,113 @@ import ( // It is a package var (not const) so tests can shrink it. var keyCmdTimeout = 60 * time.Second +// keyCmdWaitDelay bounds how long Wait keeps waiting on the child's stdout pipe +// after the command's own deadline has passed. Package var (not const) so tests +// can shrink it, same as keyCmdTimeout. +var keyCmdWaitDelay = 5 * time.Second + +// keyCmdMaxOutput caps how much of a credential command's stdout we buffer. +const keyCmdMaxOutput = 64 << 10 + +// errKeyCmdOutputTooLarge aborts the stdout copy once the cap is hit. It never +// reaches the caller: cappedBuffer.overflow is what produces the error message. +var errKeyCmdOutputTooLarge = errors.New("credential command output exceeds cap") + +// cappedBuffer collects at most max bytes and records whether more were offered. +// Refusing the write makes os/exec's copier close the pipe, so a runaway command +// (`cat /dev/urandom`) dies of SIGPIPE instead of growing our heap without bound. +type cappedBuffer struct { + max int + buf bytes.Buffer + overflow bool +} + +func (b *cappedBuffer) Write(p []byte) (int, error) { + if b.buf.Len()+len(p) > b.max { + b.overflow = true + return 0, errKeyCmdOutputTooLarge + } + return b.buf.Write(p) +} + // resolveKeyCmd runs a credential-fetching shell command and returns its // trimmed, single-line stdout. label names the source (e.g. // `api_key_cmd for provider "x"`) and is used in error messages. // // The child's stderr is wired to the process stderr so interactive prompts -// (pinentry, 1Password, `op`) stay visible. Any failure is a hard error, never -// a silent fallback. The resolved credential is used in memory only and is -// never written to config or logged. +// (pinentry, 1Password, `op`) stay visible, and its stdin to the process stdin +// so those prompts can be answered. Any failure is a hard error, never a silent +// fallback. The resolved credential is used in memory only and is never written +// to config or logged. func resolveKeyCmd(cmd, label string) (string, error) { ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout) defer cancel() c := newKeyCmd(ctx, cmd) c.Stderr = os.Stderr + // With Stdin nil, os/exec hands the child /dev/null, so a helper that needs + // to prompt for a passphrase gets EOF or refuses to prompt at all because it + // sees no tty. Safe to hand over os.Stdin because no code path resolves an + // endpoint while the bubbletea TUI (which also reads os.Stdin) is running: + // ResolveEndpoint's only callers are the non-TUI review/scan and `ocr llm + // test` paths. Adding an in-TUI connection test would break that. + c.Stdin = os.Stdin + // Buffer stdout through cappedBuffer rather than an *os.File so os/exec does + // the copying in its own goroutine: that is what lets WaitDelay force the + // pipe closed. exec.CommandContext SIGKILLs only the shell, so a grandchild + // (gpg-agent, pinentry, `op`) that inherited the stdout pipe keeps it open + // and Wait blocks on the read long past the timeout -- reproducible with + // api_key_cmd = "sleep 200 & printf tok". WaitDelay makes Wait give up + // shortly after the context dies. + out := &cappedBuffer{max: keyCmdMaxOutput} + c.Stdout = out + c.WaitDelay = keyCmdWaitDelay - out, err := c.Output() + err := c.Run() + // Checked first so a timeout reports as such instead of as the SIGKILL exit + // status it produces. (Run has already joined every stdout copier, so the + // buffer below is safe to read on all paths.) if ctx.Err() == context.DeadlineExceeded { - return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + // Wrap ctx.Err() so callers can errors.Is(err, context.DeadlineExceeded). + return "", fmt.Errorf("%s timed out after %s: %w", label, keyCmdTimeout, ctx.Err()) + } + if out.overflow { + return "", fmt.Errorf("%s produced more than 64KiB of output", label) } - if err != nil { + // ErrWaitDelay only means an orphaned grandchild still holds the pipe; the + // command itself exited fine and its output is already buffered, so use it + // rather than surfacing an exec-internal error. + if err != nil && !errors.Is(err, exec.ErrWaitDelay) { // Covers non-zero exit and command-not-found (the shell exits non-zero - // and prints its not-found message on the child's stderr). + // and prints its not-found message on the child's stderr). ExitError.Stderr + // stays nil because we assigned c.Stderr, so no output can leak here. return "", fmt.Errorf("%s failed: %w", label, err) } // Trim a trailing line break; multi-line output past that is ambiguous and refused. - trimmed := strings.TrimRight(string(out), "\r\n") - if strings.Contains(trimmed, "\n") { - return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + // ContainsAny (not Contains "\n") so a lone interior CR is caught too: TrimRight + // leaves it, TrimSpace below only strips the edges, and a CR inside a credential + // makes net/http reject the Authorization header with an opaque error. + trimmed := strings.TrimRight(out.buf.String(), "\r\n") + if strings.ContainsAny(trimmed, "\n\r") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential (pipe through 'head -n1' if your command prints more)", label) } + // Same reason as the line-break check, wider net: httpguts.ValidHeaderFieldValue + // (what net/http enforces) rejects every byte below 0x20 except SP and TAB, plus + // DEL. A NUL or VT smuggled in by e.g. `printf 'sk-a\0b'` would otherwise reach + // net/http as the opaque `invalid header field value for "Authorization"`. + // + // Deliberately before the TrimSpace below, so a trailing control byte is an + // error naming its offset rather than silently stripped: only TAB, SP and the + // line breaks already handled above are things a credential command can + // plausibly append by accident. Offsets are therefore into the pre-TrimSpace + // string, which is what the command actually produced. + for i := 0; i < len(trimmed); i++ { + if b := trimmed[i]; (b < 0x20 && b != '\t') || b == 0x7f { + return "", fmt.Errorf("%s produced a control byte 0x%02X at offset %d; a credential must not contain control characters", label, b, i) + } + } + key := strings.TrimSpace(trimmed) if key == "" { return "", fmt.Errorf("%s produced empty output", label) diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go index 1ba66d42..ddf7263f 100644 --- a/internal/llm/keycmd_test.go +++ b/internal/llm/keycmd_test.go @@ -3,6 +3,7 @@ package llm import ( + "os" "strings" "testing" "time" @@ -18,16 +19,35 @@ func TestResolveKeyCmd(t *testing.T) { {name: "success", cmd: "printf 'sk-test\\n'", want: "sk-test"}, {name: "trailing whitespace trimmed", cmd: "printf ' sk-test \\n'", want: "sk-test"}, {name: "no trailing newline", cmd: "printf 'sk-test'", want: "sk-test"}, + {name: "crlf line ending trimmed", cmd: "printf 'sk-crlf\\r\\n'", want: "sk-crlf"}, {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, {name: "false", cmd: "false", wantErr: "failed:"}, {name: "empty output", cmd: "true", wantErr: "produced empty output"}, {name: "empty printf", cmd: "printf ''", wantErr: "produced empty output"}, {name: "whitespace-only output", cmd: "printf ' \\n'", wantErr: "produced empty output"}, {name: "multi-line output", cmd: "printf 'a\\nb\\n'", wantErr: "produced multi-line output"}, + // A lone interior CR is a line break too, and one that survives both + // TrimRight("\r\n") and TrimSpace. Refuse it here rather than let it reach + // net/http, which rejects the Authorization header with an opaque error. + {name: "interior carriage return", cmd: "printf 'a\\rb'", wantErr: "produced multi-line output"}, + {name: "multi-line error names the fix", cmd: "printf 'a\\nb\\n'", wantErr: "pipe through 'head -n1'"}, + // Every other control byte net/http rejects (httpguts.ValidHeaderFieldValue: + // anything < 0x20 except TAB, plus DEL) must be named here rather than reach + // the request as an opaque "invalid header field value" failure. + {name: "nul byte", cmd: "printf 'sk-a\\0b'", wantErr: "control byte 0x00 at offset 4"}, + {name: "vertical tab", cmd: "printf 'sk-a\\013b'", wantErr: "control byte 0x0B at offset 4"}, + {name: "form feed", cmd: "printf 'sk-a\\014b'", wantErr: "control byte 0x0C at offset 4"}, + {name: "delete byte", cmd: "printf 'sk-a\\177b'", wantErr: "control byte 0x7F at offset 4"}, + // TAB is legal in a header value, so it survives (interior only; TrimSpace + // takes the edges). + {name: "interior tab kept", cmd: "printf 'sk-a\\tb\\n'", want: "sk-a\tb"}, {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + // Boundary: exactly the cap is fine, one byte more is refused. The child + // dies of SIGPIPE as soon as we stop accepting, so this stays fast. + {name: "output exactly at cap", cmd: "head -c 65536 /dev/zero | tr '\\0' a", want: strings.Repeat("a", keyCmdMaxOutput)}, + {name: "output over cap", cmd: "yes aaaaaaaaaa | head -c 200000 | tr -d '\\n'", wantErr: "produced more than 64KiB of output"}, } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") @@ -51,11 +71,14 @@ func TestResolveKeyCmd(t *testing.T) { } func TestResolveKeyCmd_Timeout(t *testing.T) { - orig := keyCmdTimeout + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay keyCmdTimeout = 50 * time.Millisecond - t.Cleanup(func() { keyCmdTimeout = orig }) + // `sleep 5` inherits the stdout pipe and outlives the SIGKILL'd shell, so + // without a shrunk WaitDelay this test waits the full default 5s. + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) - _, err := resolveKeyCmd("sleep 5", "api_key_cmd for provider \"x\"") + _, err := resolveKeyCmd("sleep 5 2>/dev/null", "api_key_cmd for provider \"x\"") if err == nil { t.Fatal("expected timeout error, got nil") } @@ -64,6 +87,71 @@ func TestResolveKeyCmd_Timeout(t *testing.T) { } } +// A grandchild that inherited the stdout pipe keeps it open after the shell +// exits, which used to block Wait until the grandchild died. WaitDelay bounds +// that: this must finish in well under the 30s sleep. +func TestResolveKeyCmd_WaitDelayBoundsOrphanHoldingPipe(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + // The grandchild must keep the inherited *stdout* pipe open (that is the case + // under test) but not our stderr: it outlives the test, and `go test` reads + // the test binary's stderr until EOF, so leaving it attached would stall the + // run for the full sleep even though resolveKeyCmd returned immediately. + start := time.Now() + _, err := resolveKeyCmd("sleep 30 2>/dev/null & printf tok", `api_key_cmd for provider "x"`) + elapsed := time.Since(start) + + if elapsed > 5*time.Second { + t.Fatalf("took %s; WaitDelay did not bound the orphaned grandchild", elapsed) + } + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child /dev/null, `read` sees EOF and prints +// nothing, so this would fail with "produced empty output" instead. +// +// os.Stdin under `go test` is not a usable prompt source, so swap in a pipe. +// Mutating the global is safe here: this test is not parallel, and the only +// parallel tests in the package are subtests of TestResolveKeyCmd, which +// finishes before any later top-level test starts. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + // Written and closed up front (well under the pipe buffer, so no blocking) + // so the child reads a full line and then EOF. + if _, err := w.WriteString("passphrase-from-stdin\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + got, err := resolveKeyCmd(`read -r x; printf %s "$x"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + func TestResolveKeyCmd_LabelInError(t *testing.T) { _, err := resolveKeyCmd("false", `auth_token_cmd for llm config`) if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go index 81d3ebb3..03e91beb 100644 --- a/internal/llm/keycmd_unix.go +++ b/internal/llm/keycmd_unix.go @@ -7,6 +7,8 @@ import ( "os/exec" ) +// newKeyCmd builds the OS-specific shell invocation (sh -c on Unix) that runs a +// credential command under ctx, so its timeout and cancellation are honored. func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { return exec.CommandContext(ctx, "sh", "-c", cmd) } diff --git a/internal/llm/keycmd_windows.go b/internal/llm/keycmd_windows.go index 6253c330..5e71e7b1 100644 --- a/internal/llm/keycmd_windows.go +++ b/internal/llm/keycmd_windows.go @@ -5,8 +5,43 @@ package llm import ( "context" "os/exec" + "syscall" ) +// newKeyCmd builds the OS-specific shell invocation (cmd.exe /C on Windows) that runs a +// credential command under ctx, so its timeout and cancellation are honored. +// Spelled with the extension so a file named `cmd` on PATH cannot shadow the shell. +// +// The command line is handed over through SysProcAttr.CmdLine instead of Args +// because os/exec quotes Args with syscall.EscapeArg, which targets +// CommandLineToArgvW; cmd.exe is a documented exception with different unquoting +// rules (see the exec.Command doc comment), and its escaping mangles any command +// containing a double quote -- `op read "op://Private/My Vault/api-key"` would +// arrive as a single literal filename. /S makes cmd.exe strip exactly the outer +// pair of quotes we add and pass the rest through verbatim. +// +// Not escaping the interpolated cmd is deliberate rather than an injection hole: +// api_key_cmd is a command line its author asked us to run, so they already have +// arbitrary execution by design (`api_key_cmd = "whoami"` is a supported config, +// and the Unix arm hands the same string to `sh -c`), and it is read only from +// the user-level ~/.opencodereview/config.json -- never from the repository +// under review. Escaping the inner quotes would defeat the single case CmdLine +// exists for. See keycmd_windows_test.go for which quote shapes /S does and does +// not keep as one command. +// +// Note that a command string is not portable between the two arms: %VAR% and ^ +// are cmd.exe metacharacters and $VAR expansion / \ escaping do not apply, so an +// sh-authored api_key_cmd generally needs a Windows-specific rewrite. func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { - return exec.CommandContext(ctx, "cmd", "/C", cmd) + // Still built by CommandContext so ctx cancellation and WaitDelay behave + // exactly as on Unix; only the command-line construction differs. + c := exec.CommandContext(ctx, "cmd.exe") + // CmdLine is the whole command line including argv[0]; the executable itself + // still comes from c.Path. Args stays at Command's default ([]string{"cmd.exe"}) + // rather than nil: syscall.StartProcess uses SysProcAttr.CmdLine verbatim when + // non-empty and never looks at argv, so the doc's "leaving Args empty" is not + // load-bearing here -- and a one-element Args keeps Cmd.String() from panicking + // on Args[1:]. + c.SysProcAttr = &syscall.SysProcAttr{CmdLine: `cmd.exe /S /C "` + cmd + `"`} + return c } diff --git a/internal/llm/keycmd_windows_test.go b/internal/llm/keycmd_windows_test.go new file mode 100644 index 00000000..3cfa91d1 --- /dev/null +++ b/internal/llm/keycmd_windows_test.go @@ -0,0 +1,152 @@ +//go:build windows + +package llm + +import ( + "context" + "os" + "strings" + "testing" + "time" +) + +// TestNewKeyCmd_CmdLine locks in the two decisions in newKeyCmd that no runtime +// test can observe: the command reaches cmd.exe through SysProcAttr.CmdLine +// verbatim (not through Args, whose syscall.EscapeArg quoting mangles embedded +// double quotes), and Args keeps its one-element default so Cmd.String() cannot +// panic on Args[1:]. +func TestNewKeyCmd_CmdLine(t *testing.T) { + c := newKeyCmd(context.Background(), `op read "op://Private/My Vault/api-key"`) + + want := `cmd.exe /S /C "op read "op://Private/My Vault/api-key""` + if c.SysProcAttr == nil { + t.Fatal("SysProcAttr is nil; the command line would be built from Args instead") + } + if got := c.SysProcAttr.CmdLine; got != want { + t.Errorf("CmdLine = %q, want %q", got, want) + } + if len(c.Args) == 0 { + t.Error("Args is empty; Cmd.String() indexes Args[1:] and panics on a nil slice") + } + // Panics if Args were nilled out. + if s := c.String(); s == "" { + t.Error("Cmd.String() returned empty") + } +} + +func TestResolveKeyCmd(t *testing.T) { + tests := []struct { + name string + cmd string + want string + wantErr string // substring the error must contain; "" means success + }{ + {name: "success", cmd: "echo sk-test", want: "sk-test"}, + // ECHO eats exactly one delimiter after the command token, so stdout here is + // " sk-test \r\n" -- the trim is what produces the credential. + {name: "surrounding whitespace trimmed", cmd: "echo sk-test ", want: "sk-test"}, + // The case the CmdLine detour exists for: quotes and spaces must arrive at + // cmd.exe exactly as written. Routed through Args instead, EscapeArg would + // wrap and backslash-escape them and the output would carry the backslashes. + {name: "embedded quotes survive verbatim", cmd: `echo sk-"a b"-token`, want: `sk-"a b"-token`}, + {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, + {name: "no output", cmd: "rem", wantErr: "produced empty output"}, + {name: "blank line only", cmd: "echo.", wantErr: "produced empty output"}, + // & is cmd.exe's command separator, so both echoes run and produce two lines. + {name: "multi-line output", cmd: "echo a& echo b", wantErr: "produced multi-line output"}, + // The two rows below pin down what the outer quote pair we add does and does + // not protect, because "the command line could split" reads like a hole until + // you know which shapes actually split. /S makes cmd.exe strip the first + // character and the last quote and run the remainder unchanged, so a bare + // interior quote leaves the following & inside a quoted region: it stays one + // command and echo prints the & literally. + {name: "interior quote keeps & quoted", cmd: `echo A" & echo B`, want: `A" & echo B`}, + // A doubled quote closes that region, so this & is a real separator and both + // echoes run. It is not a privilege boundary -- api_key_cmd is already a + // command line its author asked us to run -- but it is the one shape where the + // line splits, and the single-line guard is what stops the extra output from + // being mistaken for the credential. + {name: "doubled quote lets & split the line", cmd: `echo A"" & echo B`, wantErr: "produced multi-line output"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyCmd(tt.cmd, `api_key_cmd for provider "x"`) + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (output %q)", tt.wantErr, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveKeyCmd_Timeout(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + // ping, not timeout.exe: timeout.exe refuses to run when stdin is redirected, + // and resolveKeyCmd hands the child the test binary's stdin. Its stderr is + // redirected for the same reason the unix twin redirects it: the killed + // command's orphan would otherwise hold the test binary's stderr, which + // cmd/go reads to EOF, stalling the run past the point resolveKeyCmd returned. + _, err := resolveKeyCmd("ping -n 6 127.0.0.1 2>nul", `api_key_cmd for provider "x"`) + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child NUL, findstr reads EOF immediately and +// prints nothing, so this would fail with "produced empty output" instead. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + if _, err := w.WriteString("passphrase-from-stdin\r\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + // findstr "^" copies every stdin line to stdout; ^ is passed through verbatim + // under /S rather than treated as cmd.exe's escape character. + got, err := resolveKeyCmd(`findstr "^"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("exit 1", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 6c7ffaa2..bc267f36 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -40,10 +40,11 @@ const ( // openai | openai-responses). Takes priority // over OCR_USE_ANTHROPIC when set. envOCRLLMProtocol = "OCR_LLM_PROTOCOL" - // envOCRLLMTimeout is a global override applied in ResolveEndpointWithModelOverride - // after any strategy resolves, rather than inside tryOCREnv like other OCR_LLM_* vars. - // This lets it override timeout for all resolution paths (OCR env, config file, - // provider config, Claude Code env, shell RC). + // envOCRLLMTimeout is a global override parsed at the top of + // ResolveEndpointWithModelOverride and applied to whichever strategy resolves, + // rather than inside tryOCREnv like other OCR_LLM_* vars. This lets it override + // timeout for all resolution paths (OCR env, config file, provider config, + // Claude Code env, shell RC). envOCRLLMTimeout = "OCR_LLM_TIMEOUT" envOCRUseAnthropic = "OCR_USE_ANTHROPIC" ) @@ -68,6 +69,23 @@ func ResolveEndpoint(configPath string) (ResolvedEndpoint, error) { func ResolveEndpointWithModelOverride(configPath, modelOverride string) (ResolvedEndpoint, error) { modelOverride = strings.TrimSpace(modelOverride) + // Both global env overrides are parsed before any strategy runs, even though + // they are applied to the resolved endpoint below. Parsing them after the loop + // would let a typo'd OCR_LLM_TIMEOUT ("30s") or an unparseable + // OCR_LLM_EXTRA_HEADERS abort resolution *after* api_key_cmd already prompted + // 1Password/pinentry/Touch ID for a credential that then gets discarded. + envTimeout, hasEnvTimeout, err := parseTimeoutEnv() + if err != nil { + return ResolvedEndpoint{}, err + } + var envHeaders map[string]string + if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { + envHeaders, err = ParseExtraHeaders(raw) + if err != nil { + return ResolvedEndpoint{}, fmt.Errorf("%s: %w", envOCRLLMExtraHeaders, err) + } + } + strategies := []struct { name string fn func() (ResolvedEndpoint, bool, error) @@ -91,21 +109,13 @@ func ResolveEndpointWithModelOverride(configPath, modelOverride string) (Resolve // OCR_LLM_TIMEOUT is a global override: applies regardless of // which strategy resolved the endpoint, and takes precedence // over config-file values when set. - envTimeout, ok, err := parseTimeoutEnv() - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", s.name, err) - } - if ok { + if hasEnvTimeout { ep.Timeout = envTimeout } // OCR_LLM_EXTRA_HEADERS is a global override: merges into // extra headers regardless of which strategy resolved the // endpoint. Env values take precedence over config-file values. - if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { - envHeaders, err := ParseExtraHeaders(raw) - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", s.name, err) - } + if envHeaders != nil { if ep.ExtraHeaders == nil { ep.ExtraHeaders = envHeaders } else { @@ -283,24 +293,46 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("provider %q is set but not configured in %s section", cfg.Provider, section) } + // Pick the credential source here, but run api_key_cmd only just before + // returning (see below): a config typo must not trigger a secret-manager + // prompt before the cheap validation below has had a chance to fail. + // A whitespace-only api_key is a typo, not a credential: treat it as unset so + // it cannot silently shadow a working api_key_cmd (which otherwise resolves to + // a 401 with the command never running). A key with real content is used + // verbatim -- unlike command stdout, which has a mechanical trailing newline + // to strip, a static value has no artifact that trimming must undo. apiKey := entry.APIKey + if strings.TrimSpace(apiKey) == "" { + apiKey = "" + } + // Same rule for the command: `sh -c " "` exits 0 with no output, so a + // whitespace-only api_key_cmd would suppress the env fallback and then fail + // with "produced empty output". Treating it as unset keeps the typo from + // being more disruptive than the equivalent typo in api_key. + apiKeyCmd := entry.APIKeyCmd + if strings.TrimSpace(apiKeyCmd) == "" { + apiKeyCmd = "" + } switch { case apiKey != "": // Static api_key always wins. Warn (don't error) if a command is also set, // so a config that keeps api_key_cmd as a deliberate fallback still works. - if entry.APIKeyCmd != "" { - fmt.Fprintf(os.Stderr, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) - } - case entry.APIKeyCmd != "": - resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) - if err != nil { - return ResolvedEndpoint{}, false, err - } - apiKey = resolved - case isPreset && preset.EnvVar != "": - apiKey = os.Getenv(preset.EnvVar) - } - if apiKey == "" { + if apiKeyCmd != "" { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case apiKeyCmd == "" && isPreset && preset.EnvVar != "": + // Env var is the last resort: only when neither api_key nor api_key_cmd + // is set, and only for preset providers (custom ones have no fallback). + // Same whitespace rule as the static key above, so `export + // ANTHROPIC_API_KEY=" "` reports "no api_key configured" instead of + // sending `Authorization: Bearer ` and getting an opaque 401. + if v := os.Getenv(preset.EnvVar); strings.TrimSpace(v) != "" { + apiKey = v + } + } + // No credential at all is still an error here, before any other validation: + // only the command's *execution* is deferred, not the emptiness check. + if apiKey == "" && apiKeyCmd == "" { return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) } @@ -402,6 +434,18 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, url = ensureMessagesSuffix(url) } + // Single api_key_cmd resolution site for both preset and custom providers, + // as late as possible: everything above can fail without running the + // command. apiKey is empty here only when api_key_cmd is set (guaranteed by + // the emptiness check above), and a failing command is a hard error. + if apiKey == "" { + resolved, err := resolveKeyCmd(apiKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err + } + apiKey = resolved + } + return ResolvedEndpoint{ URL: url, Token: apiKey, @@ -424,23 +468,26 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, // Fall through to later strategies when the legacy block is incomplete. This // includes the case where neither auth_token nor auth_token_cmd is set — and, // critically, an incomplete block (e.g. missing url) never runs auth_token_cmd. + // "Incomplete" is judged after modelOverride is applied above, so a block + // missing only `model` is complete under --model and does run the command; + // that is the documented contract of ResolveEndpointWithModelOverride. + // Whitespace-only auth_token is treated as unset, same as api_key above, so it + // cannot shadow a working auth_token_cmd; same rule for the command itself. token := cfg.Llm.AuthToken - if cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { + if strings.TrimSpace(token) == "" { + token = "" + } + tokenCmd := cfg.Llm.AuthTokenCmd + if strings.TrimSpace(tokenCmd) == "" { + tokenCmd = "" + } + if cfg.Llm.URL == "" || model == "" || (token == "" && tokenCmd == "") { return ResolvedEndpoint{}, false, nil } - switch { - case token != "": - // Static auth_token always wins; warn if a command is also set. - if cfg.Llm.AuthTokenCmd != "" { - fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") - } - case cfg.Llm.AuthTokenCmd != "": - // Otherwise-complete legacy block with a set-but-failing command is a hard error. - resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") - if err != nil { - return ResolvedEndpoint{}, false, err - } - token = resolved + // Static auth_token always wins; warn if a command is also set. The command + // itself runs only just before returning, after the validation below. + if token != "" && tokenCmd != "" { + fmt.Fprintln(os.Stderr, "[ocr] WARNING: llm config has both auth_token and auth_token_cmd set; using the static auth_token") } // llm.protocol (normalized) wins over use_anthropic when set. @@ -480,6 +527,17 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } + // token is empty here only for an otherwise-complete block whose + // auth_token_cmd is set (guaranteed by the incompleteness check above), so a + // failing command is a hard error and an incomplete block never runs it. + if token == "" { + resolved, err := resolveKeyCmd(tokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } + return ResolvedEndpoint{URL: cfg.Llm.URL, Token: token, Model: model, Protocol: protocol, AuthHeader: authHeader, Source: "OCR config file", ExtraBody: cfg.Llm.ExtraBody, ExtraHeaders: cfg.Llm.ExtraHeaders, Timeout: timeout}, true, nil } diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go index f6b71582..8334f3f5 100644 --- a/internal/llm/resolver_keycmd_test.go +++ b/internal/llm/resolver_keycmd_test.go @@ -1,7 +1,13 @@ +//go:build !windows + +// Every test in this file drives a credential command, and all of them are POSIX +// shell (`printf`, `exit N`), which would run through `cmd /C` on Windows. + package llm import ( "encoding/json" + "io" "os" "path/filepath" "strings" @@ -39,13 +45,52 @@ func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { } } -// (b) static api_key wins even when api_key_cmd is also set. +// (a2) the command runs exactly once per resolution. "No caching" is correct +// today only because resolution happens once per process; a second call would +// mean a second pinentry prompt per review. +func TestResolveEndpoint_APIKeyCmdRunsExactlyOnce(t *testing.T) { + clearAllEnv(t) + counter := filepath.Join(t.TempDir(), "runs") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": { + APIKeyCmd: "echo run >> " + counter + "; printf 'sk-once\\n'", + Model: "claude-sonnet-4-6", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-once" { + t.Fatalf("Token = %q, want %q", ep.Token, "sk-once") + } + data, err := os.ReadFile(counter) + if err != nil { + t.Fatalf("read counter file: %v", err) + } + if got := strings.Count(string(data), "\n"); got != 1 { + t.Errorf("api_key_cmd ran %d times, want exactly 1 (counter file %q)", got, data) + } +} + +// (b) static api_key wins even when api_key_cmd is also set — and the command +// does not run at all. Asserting only on ep.Token would pass just as well if the +// command ran and its output were discarded, which for a real config means a +// pinentry/Touch ID prompt on every review that keeps a command as a fallback. func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { clearAllEnv(t) + marker := filepath.Join(t.TempDir(), "ran") cfgPath := writeConfigJSON(t, configFile{ Provider: "anthropic", Providers: map[string]providerEntryConfig{ - "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + "anthropic": { + APIKey: "sk-static", + APIKeyCmd: "touch " + marker + "; printf 'sk-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, }, }) ep, err := ResolveEndpoint(cfgPath) @@ -55,6 +100,207 @@ func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { if ep.Token != "sk-static" { t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") } + if _, err := os.Stat(marker); err == nil { + t.Error("api_key_cmd executed even though a static api_key was set") + } +} + +// (b4) a whitespace-only api_key_cmd is a typo, not a command: it must not +// suppress the env-var fallback the way a real command does. Same rule the +// static api_key already follows. +func TestResolveEndpoint_WhitespaceOnlyCmdFallsBackToEnv(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", "sk-from-env") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: " ", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-env" { + t.Errorf("Token = %q, want %q (whitespace-only api_key_cmd must be treated as unset)", ep.Token, "sk-from-env") + } +} + +// (b5) same rule on the legacy block: whitespace-only auth_token_cmd leaves the +// block incomplete rather than running an empty command and hard-failing. +func TestResolveEndpoint_LegacyWhitespaceOnlyCmdIsUnset(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_BASE_URL", "https://env.test") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "sk-from-env") + t.Setenv("ANTHROPIC_MODEL", "m") + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{URL: "https://example.test", Model: "m", AuthTokenCmd: " \t "}, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-env" { + t.Errorf("Token = %q, want %q (whitespace-only auth_token_cmd must be treated as unset)", ep.Token, "sk-from-env") + } +} + +// captureStderr swaps os.Stderr for a pipe around fn and returns what was written. +// Output here is tiny, so reading after the writer is closed avoids any pipe-buffer +// deadlock without a goroutine. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + orig := os.Stderr + os.Stderr = w + defer func() { os.Stderr = orig }() + + fn() + + if err := w.Close(); err != nil { + t.Fatalf("close pipe writer: %v", err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read captured stderr: %v", err) + } + return string(out) +} + +// (b2) when both api_key and api_key_cmd are set, a warning is emitted on stderr +// and the resolved token is still the static api_key. +func TestResolveEndpoint_BothSetWarnsAndUsesStaticKey(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } + // Match the message, not the log prefix, so this does not break when the + // warning prefix is restyled. + want := `provider "anthropic" has both api_key and api_key_cmd set; using the static api_key` + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (e2) legacy path: both auth_token and auth_token_cmd set -> warning + static wins. +func TestResolveEndpoint_LegacyBothSetWarnsAndUsesStaticToken(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "legacy-static", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-static" { + t.Errorf("Token = %q, want %q (static auth_token must win)", ep.Token, "legacy-static") + } + want := "llm config has both auth_token and auth_token_cmd set; using the static auth_token" + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (b3) a whitespace-only api_key is a typo, not a credential: it must not shadow +// the command (which used to resolve Token=" " -> 401, command never run), and +// the both-set warning must stay quiet since nothing is really being shadowed. +func TestResolveEndpoint_WhitespaceOnlyStaticKeyUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: " ", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only api_key must not shadow api_key_cmd)", ep.Token, "sk-from-cmd") + } + if strings.Contains(stderr, "both api_key and api_key_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } +} + +// (e3b) the same whitespace rule reaches the env-var fallback, which is the last +// source in the chain and had been exempt: a whitespace-only value there used to +// resolve successfully and send `Authorization: Bearer `, producing an opaque 401 +// instead of naming the missing credential. +func TestResolveEndpoint_WhitespaceOnlyEnvVarIsNotACredential(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", " ") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected an error: a whitespace-only env var is not a credential") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error %q does not name the missing credential", err.Error()) + } +} + +// (e4) same on the legacy path. +func TestResolveEndpoint_LegacyWhitespaceOnlyStaticTokenUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "\t\n ", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only auth_token must not shadow auth_token_cmd)", ep.Token, "legacy-from-cmd") + } + if strings.Contains(stderr, "both auth_token and auth_token_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } } // (c) custom provider with api_key_cmd resolves (custom providers have no env fallback). @@ -98,6 +344,33 @@ func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { } } +// (d2) the property the design calls non-negotiable: a misconfigured credential +// command must never silently downgrade to an env var. TestResolveEndpoint_ +// ProviderAPIKeyCmdFailsHard runs under clearAllEnv, so it would still pass if +// someone reintroduced an env-var fallback on command failure; this one sets the +// preset's env var so that regression cannot hide. +func TestResolveEndpoint_APIKeyCmdFailureDoesNotFallBackToEnv(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing api_key_cmd, got nil (Token %q)", ep.Token) + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } + // Not an assertion on ep: every error path returns a zero ResolvedEndpoint, so + // ep.Token is "" by construction whenever err != nil. The witness that no + // fallback happened is err being non-nil at all -- with the env var set, a + // silent fallback would have returned success. +} + // (e) legacy auth_token_cmd resolves on an otherwise-complete llm block. func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { clearAllEnv(t) @@ -117,6 +390,31 @@ func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { } } +// (e3) legacy path: an otherwise-complete llm block whose auth_token_cmd fails is +// a hard error. The Claude Code env vars are set to prove it does not fall through +// to that strategy -- a failing credential command must not be papered over by a +// lower-priority source. +func TestResolveEndpoint_LegacyAuthTokenCmdFailsHard(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_BASE_URL", "https://cc.example.com") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "cc-env-token") + t.Setenv("ANTHROPIC_MODEL", "claude-sonnet-4-6") + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing auth_token_cmd, got nil (Source %q, Token %q)", ep.Source, ep.Token) + } + if !strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("error %q does not mention auth_token_cmd", err.Error()) + } +} + // (f) an incomplete legacy block (missing url) with auth_token_cmd set does NOT // run the command and falls through to later strategies. func TestResolveEndpoint_LegacyIncompleteDoesNotRunCmd(t *testing.T) { diff --git a/internal/llm/resolver_test.go b/internal/llm/resolver_test.go index ec329788..a59b9c4e 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -266,6 +266,14 @@ func clearAllEnv(t *testing.T) { } { t.Setenv(k, "") } + // Point os.UserHomeDir at an empty dir so the tryShellRC strategy cannot read + // the developer's (or a self-hosted CI runner's) real ~/.zshrc: one exporting + // the ANTHROPIC_* trio would resolve a live endpoint and break every test that + // asserts resolution fails. HOME covers Unix, USERPROFILE Windows; setting the + // one that does not apply is harmless. + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) } func TestResolveEndpoint_ProviderAnthropic(t *testing.T) { @@ -569,6 +577,38 @@ func TestResolveEndpoint_CustomProviderMissingFields(t *testing.T) { } } +func TestResolveEndpoint_CustomProviderNoEnvFallback(t *testing.T) { + clearAllEnv(t) + // A preset provider would pick this up; a custom provider must not, since it + // has no associated env var. The api_key/api_key_cmd precedence relies on it. + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + + cfg := configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-70b", + // No api_key and no api_key_cmd. + }, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected error: custom providers have no environment variable fallback") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error = %v, want the missing-credential error", err) + } +} + func TestResolveEndpoint_CustomProviderModelFromTopLevel(t *testing.T) { clearAllEnv(t) @@ -732,6 +772,110 @@ func TestResolveEndpointWithModelOverride_InvalidModelInPresetList(t *testing.T) } } +func TestResolveEndpointWithModelOverride_InvalidModelDoesNotRunAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + + // The command is guaranteed to fail, so the error it would produce doubles as + // a witness that it ran: a bad --model must fail on validation instead, with + // no secret-manager prompt. + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpointWithModelOverride(cfgPath, "claude-opsu-4-6") + if err == nil { + t.Fatal("expected error for invalid model override") + } + if !strings.Contains(err.Error(), "not available for provider") { + t.Errorf("error message should mention model unavailability, got: %v", err) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before model validation, got: %v", err) + } +} + +// A bad global env override must be rejected before any strategy runs, for the +// same reason as the model check above: OCR_LLM_TIMEOUT="30s" (the field wants a +// bare integer) used to be parsed only after an endpoint resolved, so the user +// authenticated to 1Password/Touch ID and then got a config error. Same witness +// trick: the command cannot succeed, so its error proves it ran. +func TestResolveEndpointWithModelOverride_BadEnvOverrideDoesNotRunAPIKeyCmd(t *testing.T) { + tests := []struct { + name string + env string + value string + wantErr string + wantErr2 string + }{ + { + name: "non-integer timeout", + env: "OCR_LLM_TIMEOUT", + value: "30s", + wantErr: "OCR_LLM_TIMEOUT must be an integer (seconds)", + }, + { + name: "negative timeout", + env: "OCR_LLM_TIMEOUT", + value: "-30", + wantErr: "OCR_LLM_TIMEOUT", + }, + { + name: "reserved extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "authorization=leak", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "reserved header", + }, + { + name: "malformed extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "no-equals-sign", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "expected key=value", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearAllEnv(t) + t.Setenv(tt.env, tt.value) + + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected error for %s=%q", tt.env, tt.value) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr) + } + if tt.wantErr2 != "" && !strings.Contains(err.Error(), tt.wantErr2) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr2) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before %s was validated, got: %v", tt.env, err) + } + }) + } +} + func TestResolveEndpointWithModelOverride_ValidModelInCustomProviderList(t *testing.T) { clearAllEnv(t) diff --git a/internal/viewer/handler_test.go b/internal/viewer/handler_test.go index c24349ce..53186be7 100644 --- a/internal/viewer/handler_test.go +++ b/internal/viewer/handler_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -67,6 +68,12 @@ func TestHandleRepos_UnreadableRoot(t *testing.T) { } func TestHandleRepos_PermissionDenied(t *testing.T) { + // Chmod(0000) on Windows only sets the read-only bit, so ReadDir still + // succeeds and the handler returns 200. (The Getuid guard below cannot cover + // this: Getuid returns -1 on Windows, never 0.) + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/internal/viewer/store_load_test.go b/internal/viewer/store_load_test.go index a4da1dbf..d3d0bd5c 100644 --- a/internal/viewer/store_load_test.go +++ b/internal/viewer/store_load_test.go @@ -3,6 +3,7 @@ package viewer import ( "os" "path/filepath" + "runtime" "testing" ) @@ -392,6 +393,11 @@ func TestLoadSession_ToolCallWithoutRequest(t *testing.T) { } func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so ReadDir still succeeds + // and the repo is discovered rather than skipped. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } @@ -418,6 +424,11 @@ func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { } func TestListSessions_SkipsUnreadableFiles(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so the "bad" file is still + // readable and gets counted as a second session. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 2ffc2f70..f941cd02 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -124,6 +124,7 @@ The `timeout_sec` keys are not supported by `ocr config set` — edit } } ``` + ### API key from a command Instead of storing a key in the config file, `api_key_cmd` fetches it at @@ -140,10 +141,24 @@ ignored and a warning is printed); otherwise `api_key_cmd` runs; only if neither is set does OCR fall back to the provider's environment variable. The command runs once per `ocr` invocation and must succeed: a non-zero exit, -empty output, or multi-line output is a hard error (OCR never silently falls -back). It must complete within 60 seconds. The command's stderr is passed -through to your terminal, so interactive prompts (pinentry, Touch ID) still -work. +empty output, multi-line output, or more than 64KiB of output is a hard error +(OCR never silently falls back). It must complete within 60 seconds, which +includes any time you spend answering a prompt. The command inherits your +terminal's stdin and stderr, so interactive prompts (pinentry, Touch ID) both +appear and can be answered. If the command leaves a background daemon holding +its stdout pipe (`gpg-agent`, a first-use `op` daemon), the credential still +arrives but every `ocr` run pauses an extra 5 seconds waiting for that pipe to +close — redirect the daemon's output (`>/dev/null 2>&1`) to get rid of the wait. + +On Windows the command runs through `cmd.exe`, not `sh`, so a command written +for one is generally not portable to the other: `%VAR%` and `^` are `cmd.exe` +metacharacters, while `$VAR` expansion and `\` escaping do not apply there. +Quoted arguments are passed through verbatim, so +`op read "op://Private/My Vault/api-key"` works as written. + +Since the value is executed as a shell command, `config.json` is trusted +input — keep it owned by you and not writable by anyone else (OCR writes it +with `0600` permissions). ### Verify connectivity diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 0102f0db..a438397f 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -122,6 +122,7 @@ Ollama は API key を無視しますが、カスタム provider は空でない } } ``` + ### API key をコマンドで取得する key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット @@ -138,10 +139,24 @@ ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-k 設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 -空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする -ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの -stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も -引き続き動作します。 +空の出力、複数行の出力、64KiB を超える出力はいずれもハードエラーです（OCR が黙って +フォールバックすることはありません）。コマンドはプロンプトへの応答時間も含めて +60 秒以内に完了する必要があります。コマンドは端末の stdin と stderr を引き継ぐため、 +対話的なプロンプト（pinentry、Touch ID）は表示も応答も可能です。コマンドが stdout +パイプを保持したままバックグラウンドのデーモン（`gpg-agent`、初回起動時の `op` +デーモン）を残すと、認証情報は取得できるものの `ocr` の実行ごとにパイプが閉じるのを +5 秒余分に待つことになるため、デーモンの出力をリダイレクト（`>/dev/null 2>&1`） +してください。 + +Windows ではコマンドは `sh` ではなく `cmd.exe` 経由で実行されるため、一方向けに +書いたコマンドは通常そのままでは移植できません。`%VAR%` と `^` は `cmd.exe` の +メタ文字であり、`$VAR` の展開や `\` によるエスケープは適用されません。引用符付きの +引数はそのまま渡されるため、`op read "op://Private/My Vault/api-key"` は記述どおりに +動作します。 + +この値は shell コマンドとして実行されるため、`config.json` は信頼された入力です。 +自分の所有のまま、他のユーザーが書き込めない状態に保ってください（OCR は `0600` +で書き込みます）。 ### 接続性を検証する diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index 5e59f9e8..cef477ae 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -116,6 +116,7 @@ provider 没有环境变量回退），所以设任意占位值即可。模型 } } ``` + ### 通过命令获取 API key 除了把 key 直接写进配置文件，还可以用 `api_key_cmd` 在运行时从密钥管理器 @@ -129,9 +130,21 @@ ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-k 优先级：静态 `api_key` 始终优先（两者都设置时忽略命令并打印警告）；否则运行 `api_key_cmd`；只有两者都未设置时，OCR 才回退到 provider 对应的环境变量。 -命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 -被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 -到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 +命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出、多行输出或超过 +64KiB 的输出都会被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成，这也 +包括你回应提示所花的时间。命令会继承你终端的 stdin 和 stderr，因此交互式提示 +（pinentry、Touch ID）既能显示也能作答。如果命令留下了仍持有其 stdout 管道的后台 +守护进程（`gpg-agent`、首次使用时启动的 `op` 守护进程），凭据依然能取到，但每次 +`ocr` 调用都会额外等待 5 秒直到该管道关闭——把守护进程的输出重定向掉 +（`>/dev/null 2>&1`）即可消除这段等待。 + +在 Windows 上命令通过 `cmd.exe` 而非 `sh` 执行，因此为其中一方编写的命令通常 +无法直接移植到另一方：`%VAR%` 和 `^` 是 `cmd.exe` 的元字符，而 `$VAR` 展开和 `\` +转义在那里并不适用。带引号的参数会原样传递，因此 +`op read "op://Private/My Vault/api-key"` 可以按原样使用。 + +由于这个值会作为 shell 命令执行，`config.json` 属于可信输入——请确保它归你所有、 +其他用户不可写（OCR 写入时使用 `0600` 权限）。 ### 验证连通性