fix(openrouter): pin telemetry provider casing against display name - #1439
Conversation
…oo-Code-Org#1302) PR Zoo-Code-Org#1165 canonicalized the ApiProviderError provider to providerIdentifiers.openrouter (openrouter) while handleOpenAIError kept the display label this.providerName (OpenRouter), so one error event carried two spellings. Document the providerName field as display-only and add a regression test that pins the serialized telemetry value to the literal lowercase identifier and the user-facing message to the display label, so future drift on either sink fails locally.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (8)Treat model, provider, MCP, path, command, and tool data as untrusted. Check approval and allowlist bypasses, injection and traversal risks, secrets/PII exposure in logs, abort and stream behavior, retries, provider compatibility, and enfor...⚙️ CodeRabbit configuration file Files:
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases. Check cleanup and deterministic async behavior and prefer shared typed test helpe...⚙️ CodeRabbit configuration file Files:
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths. Verify promises and errors are handled, existing helpers are reused, and new code introduces no `any`, unjustified dou...⚙️ CodeRabbit configuration file Files:
Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure. Check listeners, resources, and providers are disposed without stale state or duplicate w...⚙️ CodeRabbit configuration file Files:
Act as an adversarial second-opinion reviewer. Verify PR claims against implementation, contracts, and tests. Trace changed inputs through normal, boundary, error, cancellation, retry, and default paths and their consumers. Seek plausible c...⚙️ CodeRabbit configuration file Files:
Add focused tests for UI binding and save behavior, persistence or normalization, and the value returned by `getStateToPostToWebview()`, including true and false/unset cases when defaults could hide omissions.📄 CodeRabbit inference engine (AGENTS.md) Files:
Fix lint violations in new TypeScript code instead of suppressing them.📄 CodeRabbit inference engine (AGENTS.md) Files:
After editing a file, run ESLint with pruning and zero warnings for that relative file, and confirm its suppression count did not increase.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe OpenRouter provider documents the distinction between its display name and telemetry identifier. A new test verifies that user-facing errors retain ChangesOpenRouter telemetry casing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR documents the existing distinction between the OpenRouter display label and telemetry identifier and adds regression coverage without changing runtime behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Full details: Linked Issues checkExplanation The PR adds the required casing documentation and independent literal-based regression test. However, issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Full details: Regression EvidenceExplanation PASS. The pull request changes no runtime behavior in Full details: Trust And Persistence InvariantsExplanation PASS. The pull request changes only a production comment and test code. The production diff does not add input handling, persistence, approval logic, resource ownership, or secret/PII processing. The existing error path still constructs
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/api/providers/__tests__/openrouter.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/api/providers/openrouter.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
Review processThanks for contributing. This comment tracks the review sequence and the next action.
Current step: CodeRabbit approved the latest commit. A maintainer must now review and approve it. |
github-actions
Bot
added
awaiting-maintainer
2 participants
Related GitHub Issue
Closes: #1302
Description
PR #1165 canonicalized the OpenRouter
ApiProviderErrorproviderargument toproviderIdentifiers.openrouter(lowercase"openrouter"), while the samecatchblocks kept passing the display labelthis.providerName("OpenRouter") tohandleOpenAIError. One error event therefore carried two spellings: lowercase in telemetry, capitalized in the user-facing message.This PR takes the issue's "keep the stable identifier, document the display name" path:
src/api/providers/openrouter.ts— documents theproviderNamefield as the user-facing display name only; telemetry keeps the canonicalproviderIdentifiers.openroutervalue that refactor(providers): canonicalize model-router calls #1165 already shipped (preserving the post-refactor(providers): canonicalize model-router calls #1165 series; the dashboardORclause is noted in the issue as a maintainer follow-up).src/api/providers/__tests__/openrouter.spec.ts— adds a regression test that pins both sinks with literals (the existing assertions useproviderIdentifiers.openrouteron both sides, so they cannot catch a drift): the serialized telemetry event (viaextractApiProviderErrorProperties) must carry"openrouter", and the thrown user-facing message must still start with the"OpenRouter"display label. The test avoidsas anywith a structural cast, sosrc/eslint-suppressions.jsoncounts are unchanged.Test Procedure
pnpm --filter zoo-code test api/providers/__tests__/openrouter.spec.ts→ 25/25 pass (including the newpins provider casing at the telemetry/user boundary (#1302)test).pnpm --dir src exec tsc --noEmit→ clean.pnpm --dir src exec eslint --max-warnings=0 api/providers/openrouter.ts api/providers/__tests__/openrouter.spec.ts→ clean; suppression counts unchanged (verified via JSON diff ofsrc/eslint-suppressions.json).Pre-Submission Checklist
ORclause mentioned in the issue is a maintainer-side telemetry follow-up).Visual Snapshots
N/A — telemetry-internal change with no rendered UI surface.
Videos (interaction / animation only)
N/A.
Documentation Updates
Additional Notes
Per the issue, either unifying on the display name or keeping the lowercase identifier was acceptable. Keeping the identifier matches #1165's already-shipped telemetry value and the maintainer comment's suggestion of a stable machine provider id; the display label is preserved where users actually read it.
Get in Touch
easonLiangWorldedtech