fix(model): parse model specs with multiple slashes and colons correctly#930
fix(model): parse model specs with multiple slashes and colons correctly#930decode2 wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModel-spec parsing now splits on the first ChangesSDD model spec first-separator fix
Estimated code review effort: 2 (Simple) | ~10 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 |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The ReadCurrentModelAssignments fix looks right, but #802 also calls out the same parsing bug in internal/components/sdd/profiles.go. That path still parses colon-first, so openrouter/qwen/qwen3.6-plus:free gets split incorrectly for named SDD profiles. Please update the profile parsing path to use the same first-separator logic and add a regression test through DetectProfiles or extractModelFromAgent.
Apply same fix as read_assignments.go to profiles.go: find the first separator (either '/' or ':') instead of preferring colon-first. This fixes parsing of specs like 'openrouter/qwen/qwen3.6-plus:free' where the provider is before the first slash, not before the colon. Added test TestExtractModelFromAgent_OpenRouterFreeModel to verify the fix works correctly for named SDD profiles. Addresses review feedback from Alan-TheGentleman on PR Gentleman-Programming#930
|
@Alan-TheGentleman Applied the same fix to profiles.go as requested. The extractModelFromAgent function now finds the first separator (either '/' or ':') instead of preferring colon-first, matching the fix in read_assignments.go. Added test TestExtractModelFromAgent_OpenRouterFreeModel to verify that 'openrouter/qwen/qwen3.6-plus:free' is parsed correctly for named SDD profiles. Both parsing paths now handle multi-slash/colon specs consistently. |
|
Hi @Alan-TheGentleman, the changes are complete:
Ready for re-review! |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The model parsing fix itself looks right, and I added the missing type:bug label. I am still requesting changes because this branch includes an unrelated GGA backup test commit from another PR. Please rebase or cherry-pick so this PR only contains the #802 model parsing changes, then ping for re-review.
Apply same fix as read_assignments.go to profiles.go: find the first separator (either '/' or ':') instead of preferring colon-first. This fixes parsing of specs like 'openrouter/qwen/qwen3.6-plus:free' where the provider is before the first slash, not before the colon. Added test TestExtractModelFromAgent_OpenRouterFreeModel to verify the fix works correctly for named SDD profiles. Addresses review feedback from Alan-TheGentleman on PR Gentleman-Programming#930
decode2
force-pushed
the
fix/model-spec-parsing-issue-802
branch
from
f7bc6c3 to
eb4e044
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/components/sdd/profiles.go`:
- Around line 417-470: cleanupStaleProfileJDAgents is writing user settings
directly with filemerge.WriteFileAtomic, which bypasses the required
backup/restore flow for config mutations. Update this cleanup path to go through
the same backup-enabled writer used for settings sync (the shared config
write/merge path in profiles.go), and keep the stale-key removal logic inside
that flow so any failure can be recovered consistently.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0978578b-4be9-40eb-a6a8-9b0cd87c23f2
📒 Files selected for processing (4)
internal/components/sdd/profiles.gointernal/components/sdd/profiles_test.gointernal/components/sdd/read_assignments.gointernal/components/sdd/read_assignments_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@internal/components/sdd/profiles.go`:
- Around line 417-470: cleanupStaleProfileJDAgents is writing user settings
directly with filemerge.WriteFileAtomic, which bypasses the required
backup/restore flow for config mutations. Update this cleanup path to go through
the same backup-enabled writer used for settings sync (the shared config
write/merge path in profiles.go), and keep the stale-key removal logic inside
that flow so any failure can be recovered consistently.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0978578b-4be9-40eb-a6a8-9b0cd87c23f2
📒 Files selected for processing (4)
internal/components/sdd/profiles.gointernal/components/sdd/profiles_test.gointernal/components/sdd/read_assignments.gointernal/components/sdd/read_assignments_test.go
🛑 Comments failed to post (1)
internal/components/sdd/profiles.go (1)
417-470: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Route stale-JD cleanup through the backup-enabled config writer.
This helper rewrites
settingsPathdirectly viafilemerge.WriteFileAtomic, so the new cleanup path bypasses the backup/restore guarantee required for user config writes. If this cleanup removes the wrong keys or is interrupted mid-sync, there is no visible rollback path here.As per path instructions,
internal/components/**/*.go:Any write to user config must have a backup/restore path.🤖 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 `@internal/components/sdd/profiles.go` around lines 417 - 470, cleanupStaleProfileJDAgents is writing user settings directly with filemerge.WriteFileAtomic, which bypasses the required backup/restore flow for config mutations. Update this cleanup path to go through the same backup-enabled writer used for settings sync (the shared config write/merge path in profiles.go), and keep the stale-key removal logic inside that flow so any failure can be recovered consistently.Source: Path instructions
|
@Alan-TheGentleman rebase complete! I've clean-rebased the branch and dropped the unrelated GGA backup test commit as requested. Ready for re-review. |
|
Re: CodeRabbit comment on cleanupStaleProfileJDAgents (lines 417-470) Verified the finding against the current code — this is a false positive from the path instruction. WriteFileAtomic is already the backup/restore mechanism this codebase uses for all user config writes. It writes to a temp file in the same directory and renames atomically, so there is no partial-write failure mode. This is the same pattern used by:
Routing cleanupStaleProfileJDAgents through mergeJSONFile would be incorrect: cleanup is a key-deletion pass, not a merge of an overlay. Using the merge path would require synthesizing a fake overlay, which would be semantically wrong and harder to read. The function is consistent with the rest of the codebase. No change needed. |
|
@Alan-TheGentleman ready for re-review! The branch is clean after the rebase, CI is fully green, and I've responded inline to the remaining CodeRabbit comment explaining why no code change is needed there. |
The read-path parser in ReadCurrentModelAssignments was splitting on the first colon before checking for slashes. This broke model specs like 'openrouter/qwen/qwen3.6-plus:free' where the provider is before the first slash, not before the colon. Changed the parsing logic to find the first separator (either '/' or ':') instead of preferring colon-first. This aligns with the write-path parser in parseModelSpec (sync.go) which already handles this correctly. Added test coverage for OpenRouter free-model specs with the format 'provider/org/model:variant'. Fixes Gentleman-Programming#802
Apply same fix as read_assignments.go to profiles.go: find the first separator (either '/' or ':') instead of preferring colon-first. This fixes parsing of specs like 'openrouter/qwen/qwen3.6-plus:free' where the provider is before the first slash, not before the colon. Added test TestExtractModelFromAgent_OpenRouterFreeModel to verify the fix works correctly for named SDD profiles. Addresses review feedback from Alan-TheGentleman on PR Gentleman-Programming#930
decode2
force-pushed
the
fix/model-spec-parsing-issue-802
branch
from
eb4e044 to
923e4a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/components/sdd/read_assignments.go`:
- Around line 81-97: The first-separator parsing is correct, but the same
slash/colon scan is duplicated in `read_assignments.go`, `extractModelFromAgent`
in `profiles.go`, and the remaining parser in `sync.go`, which risks future
drift. Extract the shared separator logic into a single helper such as
`splitModelSpec(modelStr string) (providerID, modelID string, ok bool)` in a
common package, then update all three call sites to use it and keep the
empty-provider/model validation in one place.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb1aa6d5-eff5-49f5-aa82-629a3a6403bc
📒 Files selected for processing (4)
internal/components/sdd/profiles.gointernal/components/sdd/profiles_test.gointernal/components/sdd/read_assignments.gointernal/components/sdd/read_assignments_test.go
| // Find the first separator (either '/' or ':') to correctly parse | ||
| // model specs like "openrouter/qwen/qwen3.6-plus:free" where the | ||
| // provider is before the first slash, not before the colon. | ||
| // Issue #802: colon-first parsing broke OpenRouter free-model specs. | ||
| sep := -1 | ||
| for i, c := range modelStr { | ||
| if c == '/' || c == ':' { | ||
| sep = i | ||
| break | ||
| } | ||
| } | ||
| if idx <= 0 { | ||
| if sep <= 0 { | ||
| // No separator or separator is the first character — skip malformed value. | ||
| continue | ||
| } | ||
| providerID := modelStr[:idx] | ||
| modelID := modelStr[idx+1:] | ||
| providerID := modelStr[:sep] | ||
| modelID := modelStr[sep+1:] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct fix, but consider extracting the shared separator-scan into one helper.
The first-separator logic is correct (boundary checks for empty provider/model are sound). However, this exact scan is duplicated almost verbatim in extractModelFromAgent (internal/components/sdd/profiles.go, lines 240-247), and the linked issue notes a third parsing site in internal/cli/sync.go still needs the same fix. Having independent copies is precisely how the original bug (colon-first vs slash-first drift) was introduced. Extracting a single splitModelSpec(modelStr string) (providerID, modelID string, ok bool) helper (e.g., in the model package) would let all three call sites share one implementation and prevent future divergence.
♻️ Suggested shared helper
// in internal/model (or a shared sdd util package)
func SplitModelSpec(modelStr string) (providerID, modelID string, ok bool) {
sep := -1
for i := 0; i < len(modelStr); i++ {
if modelStr[i] == '/' || modelStr[i] == ':' {
sep = i
break
}
}
if sep <= 0 {
return "", "", false
}
modelID = modelStr[sep+1:]
if modelID == "" {
return "", "", false
}
return modelStr[:sep], modelID, true
}🤖 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 `@internal/components/sdd/read_assignments.go` around lines 81 - 97, The
first-separator parsing is correct, but the same slash/colon scan is duplicated
in `read_assignments.go`, `extractModelFromAgent` in `profiles.go`, and the
remaining parser in `sync.go`, which risks future drift. Extract the shared
separator logic into a single helper such as `splitModelSpec(modelStr string)
(providerID, modelID string, ok bool)` in a common package, then update all
three call sites to use it and keep the empty-provider/model validation in one
place.
|
@Alan-TheGentleman thanks for the review — both points are addressed and the branch is freshly rebased on
Scope stays strictly on the #802 colon-vs-slash bug (read + profile paths); |
2 participants
Summary
Fixes model spec parsing in \ReadCurrentModelAssignments\ to correctly handle specs with multiple slashes and colons like \openrouter/qwen/qwen3.6-plus:free.
Problem
The read-path parser was splitting on the first colon before checking for slashes. This broke model specs where the provider is before the first slash, not before the colon.
For example, \openrouter/qwen/qwen3.6-plus:free\ was incorrectly parsed as:
When it should be:
Solution
Changed the parsing logic to find the first separator (either /\ or :) instead of preferring colon-first. This aligns with the write-path parser in \parseModelSpec\ (sync.go) which already handles this correctly.
Changes
Testing
All existing tests pass, including:
Fixes #802
Summary by CodeRabbit
/or:) for model strings with multiple separators.