Skip to content

Fix #8: merge model metadata and align reasoning behavior - #9

Closed
dillonzq wants to merge 2 commits into
massiveits:mainfrom
dillonzq:fix/issue-8-model-metadata
Closed

dillonzq wants to merge 2 commits into
massiveits:mainfrom
dillonzq:fix/issue-8-model-metadata

Conversation

@dillonzq

@dillonzq dillonzq commented Sep 24, 2026 •

Copy link
Copy Markdown

Summary

  • Enrich only catalog-discovered models with matching opencode-go metadata from models.dev; retain the last good metadata when that service is unavailable.
  • Add per-model metadata overrides for thinking capabilities, context/output limits, and modalities, with field precedence: override > catalog > models.dev.
  • Validate native and translated reasoning controls against the same model capabilities. Native Claude enabled thinking budgets are checked against model bounds.
  • Publish metadata fallback and snapshot changes atomically, and keep integration tests offline.

Verification

Fixes #8

Copilot AI lite review requested due to automatic review settings September 24, 2026 10:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Three moderate findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

This PR enriches catalog models with models.dev metadata, adds per-model overrides, and aligns reasoning validation across protocols.

Changes:

  • Adds metadata fetching, caching, precedence merging, and configuration overrides.
  • Updates native and translated reasoning validation.
  • Expands documentation and test coverage.
File Summary Findings
README.md Documents metadata enrichment and overrides. —
internal/​plugin/​plugin.go Registers the new configuration field. —
internal/​plugin/​plugin_test.go Updates registration and metadata request tests. —
internal/​plugin/​integration_test.go Covers unauthenticated metadata requests. Moderate (1 vote): avoid live models.dev requests in integration tests.
internal/​plugin/​executor_test.go Mocks models.dev failures. —
internal/​config/​config.go Adds and validates metadata overrides. —
internal/​config/​config_test.go Tests override parsing and validation. —
internal/​catalog/​catalog.go Fetches, caches, and merges metadata. Moderate (1 vote): make metadata fallback and snapshot replacement atomic.
internal/​catalog/​catalog_test.go Tests metadata precedence and retention. —
internal/​adapter/​responses/​request.go Validates native Responses reasoning. —
internal/​adapter/​responses/​request_test.go Updates reasoning sentinel tests. —
internal/​adapter/​responses_parity_test.go Adds cross-protocol parity coverage. —
internal/​adapter/​messages/​request.go Validates native disabled thinking. Moderate (3 votes): also validate enabled thinking budgets against target capabilities.
internal/​adapter/​chatcompletions/​request.go Validates native Chat Completions reasoning. —

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/adapter/messages/request.go Outdated
@dillonzq

Copy link
Copy Markdown
Author

Verification for commit a869dd6957f7abb062f801af194e1a0a9ec5e48d:

  • Fork CI Test job passed. It ran go test ./... and go vet ./... against this commit.
  • Local fresh run: go test ./... -count=1 passed (exit 0; Go 1.27.1, darwin/arm64). Full output:
?   	opencode-go-cliproxyapi	[no test files]
ok  	opencode-go-cliproxyapi/internal/adapter	0.602s
ok  	opencode-go-cliproxyapi/internal/adapter/chatcompletions	0.684s
ok  	opencode-go-cliproxyapi/internal/adapter/messages	0.950s
ok  	opencode-go-cliproxyapi/internal/adapter/responses	0.679s
ok  	opencode-go-cliproxyapi/internal/adapter/shared	0.637s
ok  	opencode-go-cliproxyapi/internal/catalog	0.949s
ok  	opencode-go-cliproxyapi/internal/config	0.812s
ok  	opencode-go-cliproxyapi/internal/errclass	0.665s
ok  	opencode-go-cliproxyapi/internal/plugin	91.599s
ok  	opencode-go-cliproxyapi/internal/thinking	0.682s
?   	opencode-go-cliproxyapi/resources	[no test files]

The upstream PR workflow currently reports action_required, so its checks await repository approval; the linked fork CI run supplies a completed test result meanwhile.

@massiveits

Copy link
Copy Markdown
Owner

Thanks for the contribution, @dillonzq.

We are closing this PR because it introduces external dependencies, unrequested configuration schemas, and breaking regressions to native traffic:

1. External models.dev dependency (catalog.go)

Model discovery in this plugin is strictly authoritative from OpenCode Go via catalog-url. Calling third-party APIs (models.dev) during catalog refresh adds an unnecessary failure point and network overhead. Capability enrichment belongs in the client harness, not inside this transport plugin.

2. Configuration schema bloat (model-metadata-overrides)

The plugin relies on route-overrides for routing corrections. Adding a second schema for per-model metadata overrides (limits, thinking bounds, modalities) adds configuration complexity without addressing a core transport requirement.

3. Regression: Gating native passthrough

The PR adds strict validation to native /v1/chat/completions, /v1/responses, and /v1/messages routes. Because OpenCode Go's discovery catalog does not expose thinking metadata, validating native requests against local tables causes the plugin to intercept and reject valid client requests before they ever reach upstream. Native traffic must pass through unblocked.

4. Responses sentinel handling

Omitting the reasoning block when "none" is not explicitly declared by the model is the current intentional design (covered by TestFromChatCompletionsEffortSentinelsOmitted). Changing this requires a concrete reproduction demonstrating an upstream OpenCode Go failure.

Closing this PR in favor of keeping the plugin minimal, self-contained, and non-blocking for native routes.

@massiveits massiveits closed this Sep 24, 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.

Merge models.dev metadata, add per-model metadata overrides, and align reasoning behavior across protocols

3 participants