Skip to content

fix: completion ratio config prioritized over hard code - #3140

Closed
feitianbubu wants to merge 1 commit into
QuantumNous:mainfrom
feitianbubu:pr/030955ed9f2e0ecc111a9ef7eed0ae630035e03c
Closed

fix: completion ratio config prioritized over hard code#3140
feitianbubu wants to merge 1 commit into
QuantumNous:mainfrom
feitianbubu:pr/030955ed9f2e0ecc111a9ef7eed0ae630035e03c

Conversation

@feitianbubu

@feitianbubu feitianbubu commented Mar 6, 2026

Copy link
Copy Markdown
Member

修复gpt-5.4倍率被硬编码为8的问题
支持手动设置倍率优于硬编码

Summary by CodeRabbit

  • Bug Fixes
    • Fixed completion ratio configuration precedence to properly prioritize user-configured settings over default values.

@coderabbitai

coderabbitai Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The GetCompletionRatio function is refactored to check the completionRatioMap before evaluating hardcoded ratios, ensuring explicitly configured values take precedence over hardcoded defaults.

Changes

Cohort / File(s) Summary
Completion Ratio Precedence
setting/ratio_setting/model_ratio.go
Reordered GetCompletionRatio logic to prioritize map-configured ratios over hardcoded fallbacks, eliminating duplicate lookups and ensuring explicit configuration takes precedence.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐰 A rabbit checked the config first,
Before the defaults quenched its thirst,
"Map before the hardcoded way!"
Now precedence wins the day,
Configuration takes the fray! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: reordering logic to prioritize config values over hardcoded ratios.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
setting/ratio_setting/model_ratio.go (2)

448-452: Dead code: the if contain check is unnecessary.

Both branches return hardCodedRatio, making the conditional pointless.

♻️ Suggested simplification
-	hardCodedRatio, contain := getHardcodedCompletionModelRatio(name)
-	if contain {
-		return hardCodedRatio
-	}
-	return hardCodedRatio
+	hardCodedRatio, _ := getHardcodedCompletionModelRatio(name)
+	return hardCodedRatio
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@setting/ratio_setting/model_ratio.go` around lines 448 - 452, The code in
model_ratio.go calls getHardcodedCompletionModelRatio(name) and then checks
contain but returns hardCodedRatio in both branches; remove the redundant
conditional and simplify by returning hardCodedRatio directly after the call to
getHardcodedCompletionModelRatio(name), eliminating the unused boolean contain
and the needless if statement.

440-447: Consider removing the redundant "/" check.

The check at lines 440-444 is now redundant. The new unconditional map lookup at lines 445-447 will find the ratio regardless of whether the name contains "/".

♻️ Suggested simplification
-	if strings.Contains(name, "/") {
-		if ratio, ok := completionRatioMap.Get(name); ok {
-			return ratio
-		}
-	}
 	if ratio, ok := completionRatioMap.Get(name); ok {
 		return ratio
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@setting/ratio_setting/model_ratio.go` around lines 440 - 447, The existing
double lookup around completionRatioMap is redundant: remove the conditional
block that checks if strings.Contains(name, "/") and its inner
completionRatioMap.Get(name) branch, and keep a single unconditional lookup
using completionRatioMap.Get(name) (referencing the name variable and
completionRatioMap.Get) to return the ratio if present; ensure no other logic
depended on the removed strings.Contains(name, "/") branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@setting/ratio_setting/model_ratio.go`:
- Around line 448-452: The code in model_ratio.go calls
getHardcodedCompletionModelRatio(name) and then checks contain but returns
hardCodedRatio in both branches; remove the redundant conditional and simplify
by returning hardCodedRatio directly after the call to
getHardcodedCompletionModelRatio(name), eliminating the unused boolean contain
and the needless if statement.
- Around line 440-447: The existing double lookup around completionRatioMap is
redundant: remove the conditional block that checks if strings.Contains(name,
"/") and its inner completionRatioMap.Get(name) branch, and keep a single
unconditional lookup using completionRatioMap.Get(name) (referencing the name
variable and completionRatioMap.Get) to return the ratio if present; ensure no
other logic depended on the removed strings.Contains(name, "/") branch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fb0bedbf-acec-4c9d-97b6-438d832fcd67

📥 Commits

Reviewing files that changed from the base of the PR and between f0e938a and 05a4314.

📒 Files selected for processing (1)
  • setting/ratio_setting/model_ratio.go

@Calcium-Ion Calcium-Ion closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants