fix(sdd): parse model spec at first separator and consolidate duplicated parsers

[Basparin]

🔗 Linked Issue

Closes #802 (read-path parse bug — scoped per review)

Refs #260 (the catalog/provider write path that produces the wrong prefix remains open there)

---

🔄 Continuation Notice

This PR continues and supersedes #242 by @ACDPDEV, who explicitly handed off the work in #242 (comment) (university commitments prevented him from landing the review suggestions himself).

Full credit for the original fix goes to @ACDPDEV — his commit 28c3378 is preserved verbatim with original author metadata intact. This PR applies @Alan-TheGentleman's two review suggestions from #242 on top:

1. Shared SplitProviderModel helper — consolidates three identical copies of the first-separator logic. This matches @ACDPDEV's final ask in the handoff: "abstraer lo que se repite en una función".
2. Regression test for OpenRouter /free format + dedicated coverage for extractModelFromAgent, which previously had no direct tests.

Once this PR lands, #242 can be closed — the full contribution (with review feedback applied) lives here.

---

🏷️ PR Type

• type:bug — Bug fix (non-breaking change that fixes an issue)
• type:feature
• type:docs
• type:refactor
• type:chore
• type:breaking-change

---

📝 Summary

• Fixes OpenRouter free-model parsing where openrouter/qwen/qwen3.6-plus:free was split at : before the leading /, producing a broken ProviderID=openrouter/qwen/qwen3.6-plus, ModelID=free.
• Consolidates three identical copies of the provider/model split logic into a single exported SplitProviderModel helper in the model package.
• Adds regression coverage for the OpenRouter format and fills the coverage gap around extractModelFromAgent.

---

📂 Changes

File / Area	What Changed
internal/model/split.go	New: exported SplitProviderModel(spec) (provider, model, ok) helper
internal/model/split_test.go	New: 11 cases including OpenRouter regression and malformed inputs
internal/cli/sync.go	parseModelSpec now delegates to the shared helper
internal/components/sdd/profiles.go	extractModelFromAgent now delegates to the shared helper
internal/components/sdd/read_assignments.go	Inline parser replaced with a call to the shared helper
internal/components/sdd/profiles_test.go	Adds TestExtractModelFromAgent — no previous coverage
internal/components/sdd/read_assignments_test.go	Adds TestReadCurrentModelAssignmentsOpenRouterFreeSuffix regression test

Net: +255 / −41, 7 files. No behavior change in the happy path — existing TestReadCurrentModelAssignments* suite still passes unchanged.

---

🧪 Test Plan

Unit Tests

go test ./internal/model/... ./internal/cli/... ./internal/components/sdd/...

E2E Tests (Docker required)

Not run

Pre-existing test failures in the Windows sandbox around unique-names-generator / bun install are unrelated to this PR — they're tracked in a separate PR (#372).

• Unit tests pass on affected packages
• E2E tests pass (cd e2e && ./docker-test.sh)
• Parsing logic verified via new unit tests across edge cases

---

🤖 Automated Checks

Check	Status	Description
Check Issue Reference	⏳	`Closes #802 (read-path parse bug — scoped per review)

Refs #260 (the catalog/provider write path that produces the wrong prefix remains open there)| | Check Issue Hasstatus:approved| ⏳ | Verified: #260 carriesstatus:approved| | Check PR Hastype:*Label | ⏳ | Requestingtype:bug` |
| Unit Tests | ⏳ | CI will validate |
| E2E Tests | ⏳ | CI matrix will validate |

---

✅ Contributor Checklist

• PR is linked to an issue with status:approved (Configure Opencode Models writes invalid model ID prefix (opencode/ instead of openrouter/) #260)
• I have added the appropriate type:* label to this PR (pending maintainer — external contributors cannot apply labels)
• Unit tests pass
• My commits follow Conventional Commits format
• My commits do not include Co-Authored-By trailers

---

💬 Notes for Reviewers

• Original author preserved: first commit (28c3378) is @ACDPDEV's verbatim, author metadata intact. GitHub should surface him as a co-contributor via commit history.
• Two new commits on top apply @Alan-TheGentleman's review from fix(sdd): parse model string on first separator to fix OpenRouter /free bug #242: refactor(model): extract SplitProviderModel helper... and test(sdd): add OpenRouter free suffix regression coverage.
• Helper placement: chose internal/model because ModelAssignment already lives there and both cli and sdd packages import it — zero import-cycle risk.
• Closing fix(sdd): parse model string on first separator to fix OpenRouter /free bug #242: once this lands, fix(sdd): parse model string on first separator to fix OpenRouter /free bug #242 is safe to close. Thanks to @ACDPDEV for the clean original fix and for trusting the continuation.

Summary by CodeRabbit

• Refactor

  • Centralized provider/model identifier parsing for consistent handling across the system.
  • Improved parsing robustness for complex and legacy model specification formats.

• Tests

  • Added unit and regression tests covering valid, invalid, and uncommon model identifier formats to prevent regressions.

[ACDPDEV]

Gracias por la mención, estoy ansioso por revisar esta PR

[Alan-TheGentleman]

This is a good candidate to merge after a current CI pass.

The parser extraction is scoped and the regression coverage hits the OpenRouter /free case, but the branch has old validation noise in the history. Please refresh against current main so we merge a clean branch.

[Alan-TheGentleman]

Looks good. The shared parser fixes the OpenRouter /free regression without widening behavior, and the new tests cover the mixed slash/colon cases that matter.

[Alan-TheGentleman]

CI is blocked on TestReadCurrentModelAssignmentsOpenRouterFreeSuffix: sdd-orchestrator is missing from the parsed assignments after the latest main update. Please update the parser or test fixture so OpenRouter /free specs still keep the phase assignment. Once that passes, this one is back in merge shape.

[Basparin]

@Alan-TheGentleman pushed the fix you flagged → commit f972839.

Fix

The regression test was asserting got["sdd-orchestrator"] but, after the alias-normalization that landed in main, ReadCurrentModelAssignments now maps the legacy sdd-orchestrator config key to gentle-orchestrator in the returned map. Updated the assertion accordingly while keeping the legacy key in the JSON fixture — that way the test now exercises both invariants in a single real-world scenario (un-synced config + first-separator split). Production code untouched.

Local validation

go test ./internal/components/sdd/... ./internal/model/...

All relevant tests pass:

• TestReadCurrentModelAssignmentsOpenRouterFreeSuffix ✅ (the one you flagged)
• All other TestReadCurrentModelAssignments* (8 tests) ✅
• TestSplitProviderModel (11 cases) ✅
• TestExtractModelFromAgent (9 cases) ✅

Remaining CI red is not from this PR

CI Unit Tests is still failing on the latest run (job 75265388060), but the failing test is TestRunInstallEngramForPiAndOpenCodeProvisionsBothMCPTargets in internal/cli/run_integration_test.go — error: Pi requires the pi executable in PATH.

Evidence this is upstream, not this PR:

1. The test doesn't exist on this branch — git grep -l TestRunInstallEngramForPiAndOpenCodeProvisionsBothMCPTargets returns nothing on fix/260-openrouter-free-continuation. It comes in via CI's auto-merge with upstream/main.
2. This branch doesn't touch internal/cli/ or internal/agentbuilder/ — git log upstream/main..HEAD -- internal/agentbuilder/ is empty.
3. Main itself is red on the same test — the last two pi commits on main (f315c05 fix(pi): write packages as array, d1027c4 fix(pi): wire engram mcp adapter) both failed CI with the same error. Runs: 25641715595, 25641492770.

Looks like the Pi test needs either a skip when pi isn't on PATH or the binary installed in the runner. Once main is green again, this PR should pass without further changes from my side. Happy to help with the Pi test fix too if useful — let me know.

[Alan-TheGentleman]

@Basparin — heads-up: this PR's Unit Tests failure is TestRunInstallEngramForPiAndOpenCodeProvisionsBothMCPTargets. That test was rewritten in main by PR #660 (fix(agents/pi): use positional pnpm dlx argument, landed in v1.31.0) to expect the new pnpm dlx gentle-engram@X pi-engram init form instead of the old pnpm dlx --package ... form.

Your branch still expects the old form, so the CI run uses the pre-#660 assertion and fails.

Fix: rebase against current main — the conflict should resolve cleanly (your changes are in different files than what #660 touched, but the integration test now has the new expectation). After the rebase, this specific failure goes away.

Pseudocode:

git fetch upstream main
git rebase upstream/main
# resolve any conflicts (likely none for this PR's scope)
git push --force-with-lease

The other PR you have open (and #371 + #374 + #372) all show the same failure for the same reason — same rebase fixes all three. Thanks for sticking with these.

[Alan-TheGentleman]

Thanks for picking this up and preserving @ACDPDEV's original commit — the refactor is clean and the new tests are solid. The multi-slash openrouter/anthropic/claude-sonnet-4 case and the :free regression are exactly right, and everything passes locally with no conflicts against main.

One blocker before merge: this fixes the wrong layer of #260.

The issue's symptom is the TUI writing opencode/qwen3.6-plus-free into opencode.json. That string is built at inject.go:1884 from assignment.FullID() (ProviderID + "/" + ModelID), and ProviderID comes from state.SelectedProvider in model_picker.go:272 — i.e. the model is being offered under the wrong provider in the catalog (the issue's own "Additional finding": it shows under OpenCode Zen but only works under OpenRouter).

Your change only fixes parsing model strings read back from config — a real and worthwhile bug — but it doesn't stop the bad prefix from being written. So Closes #260 would auto-close an issue whose main symptom is still present.

Could you either:

• (a) narrow the scope: change Closes #260 → Refs #260 (and link the colon-first parse bug it actually fixes), or
• (b) extend the PR to address the catalog/provider-mapping write path so #260's repro actually produces openrouter/...:free?

Small nit: gofmt on split_test.go to align the struct columns.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR consolidates three instances of manual provider/model string splitting into a single, tested SplitProviderModel function. The refactor centralizes parsing logic that splits on the first / or : separator, with regression coverage for OpenRouter models containing both characters.

Changes

Model Provider/Model Specification Parsing

Layer / File(s)	Summary
SplitProviderModel function and test suite internal/model/split.go, internal/model/split_test.go	Introduces SplitProviderModel(spec string) that splits at the first / or : and validates both provider and model parts are non-empty. Comprehensive tests cover valid colon/slash splits, an OpenRouter regression where models contain both separators (split at first /), and invalid-spec cases.
CLI parseModelSpec migration internal/cli/sync.go	parseModelSpec replaces manual splitting with model.SplitProviderModel, using the ok flag to validate parsing and return consistent error messages.
Profile detection extractModelFromAgent migration internal/components/sdd/profiles.go, internal/components/sdd/profiles_test.go	extractModelFromAgent delegates provider/model parsing to SplitProviderModel, returning an empty assignment on failure. New TestExtractModelFromAgent table-driven test validates parsing with multiple separator formats and OpenRouter free-suffix regression case.
Current assignments ReadCurrentModelAssignments migration internal/components/sdd/read_assignments.go, internal/components/sdd/read_assignments_test.go	ReadCurrentModelAssignments adopts SplitProviderModel for parsing agent model strings, eliminating manual index logic and removing the strings import. New TestReadCurrentModelAssignmentsOpenRouterFreeSuffix regression test verifies correct parsing and legacy orchestrator key normalization with OpenRouter models containing both / and :.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: consolidating duplicate model spec parsers into a single shared implementation that correctly splits at the first separator.
Linked Issues check	✅ Passed	The PR directly addresses issue #260 by implementing SplitProviderModel to parse model specs correctly (splitting at first / or :) and consolidating duplicate parsers across three files, fixing the OpenRouter free-model parsing issue.
Out of Scope Changes check	✅ Passed	All changes are in-scope: new helper function, three uses of the helper to replace duplicated parsing logic, and regression/unit tests for the specific fix.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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

[Basparin]

@Alan-TheGentleman review del 29-may atendida:

1. Scope ajustado a opción (a) ✅ — Closes #260 → Refs #260 en el body. Tenés razón: esto arregla el read path (parseo colon-first), no el write path del catálogo (model_picker.go:272 → inject.go:1884) que produce el prefijo malo. Configure Opencode Models writes invalid model ID prefix (opencode/ instead of openrouter/) #260 queda abierto para ese fix — puedo tomarlo como PR aparte si te sirve.
2. gofmt en split_test.go ✅ — commit 5249fc3.
3. Rebase sobre main actual ✅ — sin conflictos, historia limpia.

Tests del scope: internal/model + TestReadCurrentModelAssignments* + TestSplitProviderModel + TestExtractModelFromAgent — todos PASS.

[Basparin]

Process note: the Check Issue Reference gate only accepts closes|fixes|resolves keywords, so plain Refs #260 fails CI. Following your option (a) — "link the colon-first parse bug it actually fixes" — I filed #802 scoped to the read-path parse bug and pointed Closes there; #260 stays open for the write path. #802 needs status:approved for the gate to go green — your call.