Repository navigation
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I checked this by calling Variant.resolve directly in a scratch test. A full CLI run against a capture server failed to start from source on both base and this PR, so this is not verified end to end. On the base, a Bedrock application inference-profile ARN with family: "claude-sonnet" gets the Nova reasoningConfig shape. With this change it gets the Claude shapes for effort, toggle and budget. The catalog id us.anthropic.claude-sonnet-4-5… and a non-Claude ARN produce the same output as before. Variant tests and the core typecheck pass.
Main concerns:
- An ARN carries no version, so ARN-backed Claude models always get
thinking: {type: "adaptive"}on effort variants. Claude 4.5 and older don't use adaptive thinking. The same model by catalog id gets onlyoutput_config. The real-profile check in #53756 used a Sonnet 4.5 profile, so this case is likely common (details inline). - The two new tests are near-copies and could be one.
| const bedrockConverse: Protocol = (model, support) => { | ||
| const id = modelID(model) | ||
| const claude = id.includes("anthropic") | ||
| const claude = id.includes("anthropic") || model.family === "claude" || model.family?.startsWith("claude-") |
There was a problem hiding this comment.
With an opaque ARN, claudeInfo(model) can't parse a version. So manual is false, and effort variants always add thinking: ADAPTIVE_THINKING (I checked: the ARN + claude-sonnet case gives thinking: adaptive + output_config). The same model by catalog id (us.anthropic.claude-sonnet-4-5…) gets only output_config: {effort}, because 4.5 and older don't use adaptive thinking. The real-profile check in #53756 used a Sonnet 4.5 profile, so this path may still be rejected for many users. It's an improvement over reasoningConfig, but please consider adding a test for the older-model case and noting the limitation. The catalog-id-as-key config from the issue still covers those models.
| ]) | ||
| }) | ||
|
|
||
| test("recognizes the bare Claude family for Bedrock inference profile ARNs", () => { |
There was a problem hiding this comment.
This test only differs from the one above in the family value. Could you merge them into one test that loops over ["claude-sonnet", "claude"]? It would be shorter with the same coverage. A non-Claude family on an ARN (e.g. nova) that still gets reasoningConfig would be a useful extra check.
kimnamu
left a comment
kimnamu
left a comment
There was a problem hiding this comment.
Thanks for picking up the reasoning half of #53756 and for folding the bot's test notes into the second commit; one loop over both family spellings reads well.
I sent the additionalModelRequestFields each side generates to real application inference profiles (us-east-1, created for this check and deleted after). The effort fix holds on Claude 4.6 and newer. Sonnet 4.5 and Haiku 4.5 reject both Claude effort spellings, so the budget path is the one they can use, and this PR fixes that path too.
| Profile backed by | Variant | dev | this PR |
|---|---|---|---|
| Sonnet 4.6 | high (effort) |
400 reasoningConfig: Extra inputs are not permitted |
200 |
| Opus 5.5 | high (effort) |
400, same | 200 |
| Sonnet 4.5 | high (effort) |
400, same | 400 output_config.effort: Extra inputs are not permitted |
| Haiku 4.5 | high (effort) |
400, same | 400 adaptive thinking is not supported on this model |
| Sonnet 4.5 | high (budget) |
400 reasoningConfig: Extra inputs are not permitted |
200, reasoningContent + text |
| Haiku 4.5 | high (budget) |
400, same | 200, reasoningContent + text |
The 4.5 rejections are not ARN-specific: output_config: { effort: "high" } alone fails the same way on us.anthropic.claude-sonnet-4-5-20250929-v1:0 and us.anthropic.claude-haiku-4-5-20251001-v1:0. A family without a version can't pick one effort shape for both generations, so keeping the adaptive default seems reasonable to me.
The new test pins only the effort path. I left an optional inline suggestion that adds the budget assertion to the same loop. For 4.5-era profiles, a Claude family plus capabilities.reasoning: { supported: true, budget: {} } resolves to these budget variants, which might be worth a line next to the limitation note.
How I checked
- Merged this branch onto dev
4585c31(no conflicts).bun test test/variant.test.tsinpackages/core: 18 pass. With onlysrc/variant.tsreverted to dev: 1 fail, the ARN getsreasoningConfig. - With the suggestion applied: 18 pass and
bun typecheckclean. Withsrc/variant.tsreverted, the budget assertion receivesreasoningConfig: { type: "enabled", budgetTokens: 32000 }. compileRequestinpackages/aiturns the budget variant on an ARN model intoadditionalModelRequestFields: { thinking: { type: "enabled", budget_tokens: 16000 } }withmaxTokens: 32000. That is the body I sent.- Effort bodies sent: dev
{ reasoningConfig: { type: "enabled", maxReasoningEffort: "high" } }, this PR{ thinking: { type: "adaptive", display: "summarized" }, output_config: { effort: "high" } }. Each profile copied the matchingus.system profile. Converse, one-line prompt.
| ]) | ||
| } |
There was a problem hiding this comment.
Optional: Sonnet and Haiku 4.5 behind an ARN reject both fields above, so a configured budget is the path they can use, and nothing pins it for an ARN yet. This fails on dev with reasoningConfig: { type: "enabled", budgetTokens: 32000 } and passes on this branch.
| ]) | |
| } | |
| ]) | |
| // Sonnet and Haiku 4.5 reject both fields above, so behind an ARN they need a budget variant. | |
| input.limit = { ...input.limit, output: 64_000 } | |
| expect(resolve(Model.Info.make(input), [{ type: "budget_tokens", min: 1024 }])).toEqual([ | |
| { id: "high", settings: { thinking: { type: "enabled", budgetTokens: 16_000 } } }, | |
| { id: "max", settings: { thinking: { type: "enabled", budgetTokens: 31_999 } } }, | |
| ]) | |
| } |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Related issue: #53756 (this PR does not close the issue)
Type of change
What does this PR do?
Bedrock application inference-profile ARNs are opaque. This change uses configured family metadata (
claudeorclaude-*) to select Claude request fields instead of Nova'sreasoningConfigfallback.Limitation: family metadata does not identify a model version. For an arbitrary custom model key with an opaque ARN, this PR cannot determine whether Claude requires adaptive or manual thinking. Older model versions may still require explicit variants or using the catalog model ID as the model key, as documented in #53756.
How did you verify your code works?
cd packages/core && bun run test test/variant.test.ts— 14 passed, 0 failed, 60 expectations.cd packages/core && bun typecheck— passed.Screenshots / recordings
N/A — no UI change.
Checklist