Repository navigation
feat(providers): register GPT-5.6 models (sol, terra, luna) - #598
Conversation
Add both models to the OpenAI provider registry and the root model_registry.yaml with their 1,050,000-token context window and 128,000 max output tokens, so token budgeting uses real limits and strict model validation accepts them. Pricing is recorded as comments only; SkillSpector still does not compute cost. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: chinmay1819 <[email protected]>
Signed-off-by: chinmay1819 <[email protected]>
Feat/gpt 5.6 terra luna
Add the GPT-5.6 flagship to the OpenAI provider registry and the root model_registry.yaml with its 1,050,000-token context window and 128,000 max output tokens. Its 922K input cap sits above the 75% input budget (~787K), so context_length alone is sufficient. Pricing is recorded as comments only. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: chinmay1819 <[email protected]>
feat(providers): register gpt-5.6-sol
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed the complete three-file diff, registry lookup/default behavior, focused provider assertions, discussion, and hosted checks. The three additive entries preserve the gpt-5.4 default. Official model pages confirm 1,050,000 context / 128,000 output limits; the existing 75% input allowance stays below the documented 922,000 input limit. Pricing comments are informational and agree with the source pages.
Sources: https://developers.openai.com/api/docs/models/gpt-5.6-sol, https://developers.openai.com/api/docs/models/gpt-5.6-terra, https://developers.openai.com/api/docs/models/gpt-5.6-luna.
The new regression checks all three provider entries. All six hosted checks pass. No required code or documentation changes found. Static review only; contributor code/tests were not executed locally.
Reviewed head: 88bce9bfd5f0400781ea49bae35648d1c3a7ee8c.
Priority: P2 — Optional model availability; no default-provider or scanner behavior changes.
|
The code review remains approved at Please refresh against current main, preserve the root-registry additions, reconcile the overlapping provider entries without duplicate keys or reverting #539, and revalidate the combined registry/tests. No merge was attempted; this is a merge-readiness action item, not a duplicate code finding. |
|
@rng1995 , I will do it |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head d7811ca517386022a6e618d87d709238099c9c20 after merging current main. The root model_registry.yaml additions are preserved, and #539's broader OpenAI/Azure generation, corrected gpt-5.4 budget, gpt-5.6 alias, and gpt-6-astra entries remain.
One merge-reconciliation issue remains: the OpenAI provider YAML now declares gpt-5.6-luna, gpt-5.6-terra, and gpt-5.6-sol twice—once in #539's canonical generation block and again in this PR's pricing block. YAML loaders commonly retain only the last duplicate while stricter loaders reject duplicates, and the current lookup assertions cannot detect drift because both copies presently resolve to the same values. Remove the duplicate mappings and retain any useful pricing comments alongside the canonical entries. The added loop in test_metadata_known_model now repeats the same three lookups covered by test_metadata_flagship_generation; replace it with meaningful root-registry/unique-key coverage or remove the redundant assertions.
Five hosted checks pass; test-unit is still pending. The pending check is a merge gate, not the reason for this decision. Static review only; contributor code/tests were not executed locally.
Merging NVIDIA#539 left gpt-5.6-luna, gpt-5.6-terra, and gpt-5.6-sol mapped twice in the OpenAI provider registry: once in the canonical 1.05M generation block and again in the pricing block added here. YAML loaders disagree on duplicate keys -- most keep the last mapping, stricter ones reject the document -- so the resolved budget was loader-dependent and the two copies could drift apart unnoticed. Keep one mapping per model, the canonical ones, and move the pricing comments alongside them. That leaves a comment-only diff for this file; every mapping in it is now NVIDIA#539's. The lookup loop added to test_metadata_known_model repeated assertions that test_metadata_flagship_generation already makes, and could not have caught the duplication in the first place, since both copies resolved to the same values. Replace it with coverage that value-only assertions cannot provide: - test_each_model_is_mapped_once reads keys via yaml.compose across the root and all bundled registries. yaml.safe_load collapses a repeated key to the last one, which is why a green suite missed this. - test_root_registry_replaces_bundled_yaml exercises the repo-root file as a SKILLSPECTOR_MODEL_REGISTRY override, pinning the ids only it carries and the fact that an override replaces the bundled YAML rather than merging with it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: chinmay1819 <[email protected]>
|
Hi @rng1995 , can you please approve and merge the request ? |
|
hi @rng1995, |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @chinmay1819, thank you for your contribution to SkillSpector and for the follow-up updates — we really appreciate it!
Re-reviewed head c5de4157. Since the changes-requested review at d7811ca5, the only change to PR files is c885fed; the later commits only merge main. The PR now does three things:
- adds the three GPT-5.6 entries to the root
model_registry.yamlsample - moves the informational pricing comments onto #539's canonical OpenAI entries, with a one-mapping-per-model note (a comment-only diff in the provider file)
- adds
TestModelRegistryFiles
What I checked:
- Values: match the official model pages.
gpt-5.6-sol/terra/lunaall have 1,050,000 context, 922,000 max input and 128,000 max output, and the pricing comments are accurate. - Loader:
registry._loadis a cachedyaml.safe_load, andSKILLSPECTOR_MODEL_REGISTRYreplaces the bundled file rather than merging with it. - Budgets: the 75% input budget is 787,500, under the 922,000 cap. Output resolves to min(262,500, 128,000) = 128,000.
- Convention: putting the entries in both the root sample and the OpenAI registry follows the
gemini-3.5-flashprecedent from #7. - Merge: the head merges cleanly with current
main, including #529's new Anthropic/Bedrock registry keys. My ownyaml.composekey scan of that merge result finds no duplicate model keys in any registry. The same scan flags the threegpt-5.6-*duplicates atd7811ca5, so the new test would have caught the original problem.
Previous findings
- Duplicate
gpt-5.6-luna/terra/solmappings inproviders/openai/model_registry.yaml(inline thread at line 66): Resolved. Each model is now mapped once (lines 56/60/66), and the pricing comments sit on the canonical entries. - Redundant lookup loop added to
test_metadata_known_model: Resolved. The loop is gone. It is replaced bytest_each_model_is_mapped_once, which is parametrized over the root file and every bundled registry with a non-vacuous glob guard, and bytest_root_registry_replaces_bundled_yaml. - Merge-result revalidation after #539 (issue comment): Resolved. #539's entries, the corrected
gpt-5.4budget, thegpt-5.6alias andgpt-6-astraare all preserved. Nothing is reverted, and the head merges cleanly with currentmain.
Findings
- [Non-blocking]
tests/unit/test_providers.py:421:assert provider.get_context_length("gpt-6-astra") is Nonepins the root sample's current gap rather than loader behaviour. A later PR that syncs the sample with the bundled OpenAI registry would fail this test for a non-bug reason; the sample still lacks thegpt-5.4default and the GPT-6 entries. Replace-not-merge is better proven with atmp_pathregistry. If drift protection is the goal, assert that every model present in both the root sample andproviders/openai/model_registry.yamlhas identical budgets. - [Non-blocking] Overlap with #650: it adds its own
test_each_model_is_mapped_onceinsideTestOpenAIProvider, using the sameyaml.composeapproach. The two PRs merge cleanly with each other, which would leave two copies of the check. Whichever lands second should drop its copy. - [Non-blocking] The PR description is out of date. It still says the provider registry gains the entries and that
test_metadata_known_modelchecks them. Please update it so the squash commit message matches the final change.
Tests/CI: All six checks pass on c5de4157. mergeStateStatus is BLOCKED only by the earlier changes-requested review. This was a static review; contributor code and tests were not run locally.
Decision: Approved (reviewed head c5de41578659793538ea4fdfadf86ceb44eaa35b)
2 participants
What
Adds
gpt-5.6-sol,gpt-5.6-terraandgpt-5.6-lunato the OpenAI provider'sbundled registry (
src/skillspector/providers/openai/model_registry.yaml) andto the root
model_registry.yaml.Why
These models already worked through
SKILLSPECTOR_MODEL, but because theyweren't in the registry, SkillSpector:
LLM chunks than needed
SKILLSPECTOR_STRICT_MODEL_VALIDATION=trueHow
Registry entries only, no code changes. All three models get the values from
OpenAI's model docs:
context_lengthmax_output_tokenswindow (~787K), which stays under that cap, so no extra field is needed.
still never computes cost, as described in
docs/INFERENCE_USAGE.md.DEFAULT_MODELstaysgpt-5.4.Testing
test_metadata_known_modelnow checks the context window and max outputtokens for all three models.
make test-unit,make lintandmake format-checkall pass.--no-llmscan with strict model validation starts cleanly for each model.