[MISC] Fix adapter-ops skill cost-tracking guidance and stale paths#2163
Merged
Conversation
The skill told adapter authors that `get_provider()` drives cost calculation and pointed at `platform-service/.../helper/cost_calculation.py` as the enforcement site. That file no longer exists, and `provider` is now only a metadata column on the usage row. Cost is actually looked up from the prefixed model string that `validate_model()` emits, via `litellm.cost_per_token()` in `sdk1/audit.py` (LLM) and `sdk1/usage_handler.py` (embedding). Both call sites swallow the exception and fall back to $0, so a miss is silent. This gap let a branded OpenAI-compatible adapter pass the documented check — returning a `get_provider()` that matches `litellm_provider` exactly — while still billing $0, because `OpenAICompatibleLLMParameters` prepends `custom_openai/` and no cost-map key uses that prefix. Rewrite the guidance to verify the model prefix instead, add a base-class decision table (native LiteLLM provider -> BaseChatCompletionParameters; no priced models -> OpenAICompatibleLLMParameters), and swap the curl of upstream JSON for a snippet that queries the pinned LiteLLM in the sdk1 venv. Also repoint every documented script path from the skill's old directory name (`unstract-adapter-extension`) to `adapter-ops`; all copy-pasteable commands in SKILL.md were broken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015i26iF1GS3x55XHEjStUeP
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughUpdated adapter-ops command documentation and repository-root references, changed update-checker output handling, and replaced provider-name validation guidance with prefixed model verification focused on LiteLLM cost tracking and OpenAI-compatible adapter prefixes. ChangesAdapter operations guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/adapter-ops/SKILL.md (1)
193-197: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the accepted management action name.
manage_models.pyacceptsadd-enum, notadd; this documented command exits with an invalid-choice error.- --action add \ + --action add-enum \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/adapter-ops/SKILL.md around lines 193 - 197, Update the documented manage_models.py example to use the accepted action name add-enum instead of add, preserving the existing adapter, provider, and models arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/adapter-ops/references/provider_capabilities.md:
- Around line 151-165: Remove the instructions directing readers to curl
LiteLLM’s upstream main pricing JSON near the pricing verification guidance.
Replace them with instructions to validate model-prefix pricing against the
pinned sdk1 environment and its installed LiteLLM data, keeping the guidance
consistent with production dependencies.
In @.claude/skills/adapter-ops/SKILL.md:
- Around line 241-251: Update check_adapter_updates.py so its --json mode emits
only the json.dumps(...) payload on stdout; route the “SDK1 Adapters Path”
diagnostic to stderr or suppress it when JSON output is enabled, while
preserving the diagnostic for normal output.
- Line 359: Insert a blank line between the “Common provider names” label and
the table beginning with “Display Name” to satisfy markdownlint MD058.
---
Outside diff comments:
In @.claude/skills/adapter-ops/SKILL.md:
- Around line 193-197: Update the documented manage_models.py example to use the
accepted action name add-enum instead of add, preserving the existing adapter,
provider, and models arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c70cb48-b48e-46d1-b0cd-087c997c4613
📒 Files selected for processing (5)
.claude/skills/adapter-ops/SKILL.md.claude/skills/adapter-ops/references/adapter_patterns.md.claude/skills/adapter-ops/references/provider_capabilities.md.claude/skills/adapter-ops/scripts/init_embedding_adapter.py.claude/skills/adapter-ops/scripts/init_llm_adapter.py
Contributor
|
| Filename | Overview |
|---|---|
| .claude/skills/adapter-ops/SKILL.md | Updates adapter setup, maintenance, and cost-tracking guidance, but one documented management command still points at a missing script path. |
| .claude/skills/adapter-ops/references/adapter_patterns.md | Updates adapter pattern guidance to verify LiteLLM model-prefix cost resolution. |
| .claude/skills/adapter-ops/references/provider_capabilities.md | Updates provider capability guidance to query the pinned LiteLLM environment. |
| .claude/skills/adapter-ops/scripts/check_adapter_updates.py | Sends the SDK adapters path banner to stderr so JSON output stays parseable. |
| .claude/skills/adapter-ops/scripts/init_embedding_adapter.py | Refreshes the repo-root path comment for the renamed skill directory. |
| .claude/skills/adapter-ops/scripts/init_llm_adapter.py | Refreshes the repo-root path comment for the renamed skill directory. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
.claude/skills/adapter-ops/SKILL.md:193
**Management script missing**
The command now uses `--action add-enum`, but it still invokes `.claude/skills/adapter-ops/scripts/manage_models.py`. This PR does not add that script under `adapter-ops/scripts/`, so an adapter author following this step can still hit a file-not-found error before the model update runs. Please add the script at this path or point the example at the supported model-management command.
Reviews (4): Last reviewed commit: "[MISC] Fix invalid --action in manage_mo..." | Re-trigger Greptile
pk-zipstack
approved these changes
Jul 13, 2026
- provider_capabilities.md: replace curl-to-litellm-main verification with the pinned sdk1-venv query, matching SKILL.md — upstream main can price models the pinned version doesn't, defeating the check. - check_adapter_updates.py: send the "SDK1 Adapters Path" diagnostic to stderr so `--json` stdout is valid JSON for jq/automation. - SKILL.md: blank line before the provider-names table (markdownlint MD058). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
SKILL.md showed `--action add`, which argparse rejects (valid choices are add-enum/remove-enum/set-default/…). Corrected to `add-enum` so the documented command runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the
adapter-opsskill, which gave adapter authors incorrect guidance about cost tracking and shipped broken script paths.Why
The cost guidance was wrong
The skill said, in three places, that
get_provider()drives cost calculation, and pointed atplatform-service/src/unstract/platform_service/helper/cost_calculation.pyas the enforcement site.That file no longer exists.
provideris now just a metadata column on the usage row (sdk1/audit.py), never consulted for pricing.Cost is actually looked up from the prefixed model string that
validate_model()emits:sdk1/audit.py→cost_per_token(model=model_name)sdk1/usage_handler.py→litellm.cost_per_token(...)Both call sites catch every exception and fall back to
0.0. A miss is silent — no test fails, no build breaks. The only symptom is revenue-affecting: zero-cost usage rows.This gap has already bitten us
PR #2156 (MiniMax LLM adapter) follows the skill's documented check correctly —
get_provider()returns"minimax", which matches LiteLLM'slitellm_providerexactly — and still bills $0. It subclassesOpenAICompatibleLLMParameters, whosevalidate_model()unconditionally prependscustom_openai/, and no LiteLLM cost-map key uses that prefix:A correct
get_provider()does not, on its own, guarantee cost resolution. The skill never said so.What changed
Reframed the mandatory check from "verify the provider name" to "verify the model prefix resolves in LiteLLM's cost map", with the real call sites named.
Added a base-class decision table. If LiteLLM has a native provider for the vendor →
BaseChatCompletionParameters, emit{provider}/{model}(followOpenRouterLLMParameters). If LiteLLM prices nothing for the vendor →OpenAICompatibleLLMParameters, pinapi_base(followNvidiaBuildLLMParameters).NvidiaBuildLLMParametersis only a safe template because LiteLLM prices zeronvidia_nim/chat models (3 entries, all rerankers, all$0.0) — there is no cost to forfeit. Copying its shape for a vendor LiteLLM does price is how Add MiniMax LLM adapter #2156 went wrong.Swapped
curlof upstreammodel_prices_and_context_window.jsonfor a snippet that queries the pinned LiteLLM in the sdk1 venv — the version that actually runs.Re-scoped
get_provider()to what it genuinely controls: the static schema path (static/{get_provider()}.json, case-sensitive) and the usage row'sprovidercolumn.Repointed every documented script path from the skill's old directory name (
unstract-adapter-extension) toadapter-ops. Every copy-pasteable command inSKILL.mdwas broken.Checks
The verification snippet now documented in the skill was run as written and reproduces the bug it exists to catch:
Docs-only change to
.claude/skills/— no runtime code touched. Pre-commit hooks pass (ruff,ruff-format,markdownlint, secret scan).🤖 Generated with Claude Code
https://claude.ai/code/session_015i26iF1GS3x55XHEjStUeP