[MISC] Fix adapter-ops skill cost-tracking guidance and stale paths#2163
Open
chandrasekharan-zipstack wants to merge 1 commit into
Open
[MISC] Fix adapter-ops skill cost-tracking guidance and stale paths#2163chandrasekharan-zipstack wants to merge 1 commit into
chandrasekharan-zipstack wants to merge 1 commit into
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
Summary by CodeRabbit
WalkthroughUpdated adapter-ops command documentation and repository-root references, 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 docstrings
🧪 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
Comment on lines
+151
to
+165
| 1. `LLM._record_usage()` logs tokens — no cost math. | ||
| 2. `Audit.push_usage_data()` (LLM) / `UsageHandler` (embedding) calls | ||
| `litellm.cost_per_token(model=model_name)`, where `model_name` is the **prefixed** model | ||
| string emitted by `validate_model()`. | ||
| 3. No cost-map entry for that string → the call raises → the bare `except` records $0. | ||
|
|
||
| The `provider` value from `get_provider()` rides along into the usage row's `provider` column | ||
| but is **not** used for pricing. | ||
|
|
||
| **Example bug**: a branded OpenAI-compatible adapter emits `custom_openai/MiniMax-M3`. LiteLLM | ||
| prices `minimax/MiniMax-M3` but has no `custom_openai/` keys, so cost silently resolves to $0 — | ||
| even though `get_provider()` correctly returns `"minimax"`. | ||
|
|
||
| See `references/adapter_patterns.md` → *Model Prefix Verification* for how to choose a base | ||
| class so the prefix resolves. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale upstream pricing verification.
This file still directs readers to curl LiteLLM’s main pricing JSON at Lines [143-145], while the new guidance requires checking the pinned sdk1 environment. That can validate a prefix against data different from the version used in production.
🤖 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/references/provider_capabilities.md around lines
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.
Comment on lines
+241
to
+251
| python .claude/skills/adapter-ops/scripts/check_adapter_updates.py | ||
|
|
||
| # Check specific adapter type | ||
| python .claude/skills/unstract-adapter-extension/scripts/check_adapter_updates.py --adapter llm | ||
| python .claude/skills/unstract-adapter-extension/scripts/check_adapter_updates.py --adapter embedding | ||
| python .claude/skills/adapter-ops/scripts/check_adapter_updates.py --adapter llm | ||
| python .claude/skills/adapter-ops/scripts/check_adapter_updates.py --adapter embedding | ||
|
|
||
| # Check specific provider | ||
| python .claude/skills/unstract-adapter-extension/scripts/check_adapter_updates.py --provider openai | ||
| python .claude/skills/adapter-ops/scripts/check_adapter_updates.py --provider openai | ||
|
|
||
| # Output as JSON | ||
| python .claude/skills/unstract-adapter-extension/scripts/check_adapter_updates.py --json | ||
| python .claude/skills/adapter-ops/scripts/check_adapter_updates.py --json |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make --json output valid JSON.
check_adapter_updates.py prints SDK1 Adapters Path: ... before json.dumps(...), so the documented --json command cannot be parsed by jq or other automation. Send the diagnostic to stderr or suppress it in JSON mode.
🧰 Tools
🪛 SkillSpector (2.3.7)
[warning] 74: [PE2] Sudo/Root Execution: Commands invoke sudo or root privileges. Verify this elevated access is necessary and justified.
Remediation: Avoid sudo/root unless strictly required. Prefer least-privilege patterns. If elevation is needed, document the justification and scope.
(Privilege Escalation (PE2))
🤖 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 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.
| `custom_openai/` prefix. | ||
|
|
||
| **Common provider names:** | ||
| | Display Name | `get_provider()` Value | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required blank line before the table.
The Common provider names label is immediately followed by the table, triggering markdownlint MD058.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 359-359: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 SkillSpector (2.3.7)
[warning] 74: [PE2] Sudo/Root Execution: Commands invoke sudo or root privileges. Verify this elevated access is necessary and justified.
Remediation: Avoid sudo/root unless strictly required. Prefer least-privilege patterns. If elevation is needed, document the justification and scope.
(Privilege Escalation (PE2))
🤖 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 at line 359, Insert a blank line between
the “Common provider names” label and the table beginning with “Display Name” to
satisfy markdownlint MD058.
Source: Linters/SAST tools
Contributor
|
| Filename | Overview |
|---|---|
| .claude/skills/adapter-ops/SKILL.md | Updates skill commands and replaces stale provider-name billing guidance with model-prefix cost verification. |
| .claude/skills/adapter-ops/references/adapter_patterns.md | Expands adapter authoring guidance around validated model prefixes and base-class selection. |
| .claude/skills/adapter-ops/references/provider_capabilities.md | Updates the provider capability reference to describe cost lookup through prefixed model strings. |
| .claude/skills/adapter-ops/scripts/init_embedding_adapter.py | Updates the repo-root path comment for the renamed skill directory. |
| .claude/skills/adapter-ops/scripts/init_llm_adapter.py | Updates the repo-root path comment for the renamed skill directory. |
Reviews (1): Last reviewed commit: "[MISC] Fix adapter-ops skill cost-tracki..." | Re-trigger Greptile
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
1 participant
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