[codex] add Go runtime contract foundations#55
Merged
Conversation
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a runtime-owned contract-gap model, a typed model registry with validation and normalized lookup, and expanded token usage normalization (including reasoning tokens) with stream integration and tests. ChangesModel Registry, Token Usage, and Config Contracts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/modelregistry/models_test.go (1)
54-127: ⚡ Quick winAdd regression tests for provider-list consistency and duplicate lookup keys.
Current tests miss two high-risk contracts:
- reject entries where
Provideris not included inAPIProviders, and- detect duplicate normalized lookup keys across entries (ID/API model/alias).
🤖 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/modelregistry/models_test.go` around lines 54 - 127, Add two regression tests: (1) TestModelEntryRejectsProviderNotInAPIProviders — create a model from validModelEntry(), set ModelEntry.Provider to a provider value not present in ModelEntry.APIProviders, call model.Validate() and assert it returns an error that mentions the provider/API providers contract (reference ModelEntry.Provider, ModelEntry.APIProviders, and ModelEntry.Validate). (2) TestRegistryDetectsDuplicateNormalizedLookupKeys — create two ModelEntry instances that differ by ID/API model/alias but normalize to the same lookup key (e.g., case variants or alias vs API model), try to build the registry with NewRegistry([]ModelEntry{...}) or insert both into Registry, and assert the registry creation/insertion fails or reports a duplicate normalized key (reference NewRegistry, Registry.Get and the normalized lookup behavior) so duplicate lookup keys are detected in tests.internal/zeroruntime/provider_test.go (1)
179-206: ⚡ Quick winAdd a mixed-shape usage aggregation test to lock in correct totals.
Current coverage validates alias-only and normalized-only paths, but not a stream that mixes both. Add one mixed case to prevent regressions in
CollectStreamaggregation behavior.Suggested test case
+func TestCollectStreamAccumulatesMixedUsageShapes(t *testing.T) { + events := make(chan StreamEvent) + go func() { + defer close(events) + events <- StreamEvent{Type: StreamEventUsage, Usage: Usage{PromptTokens: 10, CompletionTokens: 4}} + events <- StreamEvent{Type: StreamEventUsage, Usage: Usage{InputTokens: 6, OutputTokens: 3, ReasoningTokens: 2}} + events <- StreamEvent{Type: StreamEventDone} + }() + + collected := CollectStream(context.Background(), events) + + if collected.Usage.EffectiveInputTokens() != 16 { + t.Fatalf("input tokens = %d, want 16", collected.Usage.EffectiveInputTokens()) + } + if collected.Usage.EffectiveOutputTokens() != 7 { + t.Fatalf("output tokens = %d, want 7", collected.Usage.EffectiveOutputTokens()) + } + if collected.Usage.ReasoningTokens != 2 { + t.Fatalf("reasoning tokens = %d, want 2", collected.Usage.ReasoningTokens) + } +}🤖 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/zeroruntime/provider_test.go` around lines 179 - 206, Add a new unit test that exercises CollectStream with a mixture of alias-shaped and normalized TokenUsage events to ensure aggregation is correct; construct one StreamEvent using a raw/alias usage shape (e.g., fields that map to EffectiveInput/Output via aliases) and another StreamEvent using NormalizeUsage(...) output, send both through the events channel followed by StreamEventDone, call CollectStream(context.Background(), events) and assert EffectiveInputTokens(), EffectiveOutputTokens(), ReasoningTokens and TotalTokens match the expected combined totals; reference CollectStream, StreamEvent, TokenUsage and NormalizeUsage to locate and implement the test.
🤖 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/modelregistry/models.go`:
- Around line 131-135: When validating a Model, besides checking each entry in
model.APIProviders with ValidRuntimeProviderKind, also enforce that when
model.APIProviders is non-empty the model.Provider value is included in that
slice; update the validation code (the loop that iterates model.APIProviders and
the similar block around the 167–176 region) to return an error if
model.Provider is not present in model.APIProviders (and ensure model.Provider
itself is validated by ValidRuntimeProviderKind where appropriate), with a clear
error message like "primary provider %q not allowed by APIProviders".
- Around line 183-193: NewRegistry currently overwrites duplicate normalized
keys silently; change register and NewRegistry to detect collisions and surface
them: modify the Registry.register(key string, entry ModelEntry) to check
registry.entries for an existing entry and return an error if an existing entry
with a different ModelEntry is present (include key, existing.ID and new
entry.ID in the error), then update NewRegistry to call register and
propagate/aggregate any errors and change its signature to return (Registry,
error) so callers can handle collisions instead of silent overwrite; update any
callers of NewRegistry accordingly.
- Around line 152-154: Replace the simple non-empty check on
cost.SourceLastVerified with a strict date parse: after trimming, attempt
time.Parse using the layout "2006-01-02" and return an error if parsing fails;
specifically update the validation around cost.SourceLastVerified in models.go
(the block that currently returns fmt.Errorf("model cost source last verified
date is required")) to instead validate and reject values that do not parse as a
YYYY-MM-DD date using time.Parse("2006-01-02", cost.SourceLastVerified).
In `@internal/zeroruntime/helpers.go`:
- Around line 68-73: The aggregation currently adds both canonical fields and
alias fields directly, causing alias-only events to be lost when later canonical
totals become non-zero; fix by normalizing each event's usage into canonical
fields before accumulation: create local normalized values from event.Usage (map
alias fields like CachedInputTokens/ReasoningTokens to the canonical
InputTokens/OutputTokens/PromptTokens/CompletionTokens as appropriate or call a
new normalize function) and then add those normalized fields into
collected.Usage (references: collected.Usage, event.Usage, and the
EffectiveInputTokens()/EffectiveOutputTokens() consumers).
---
Nitpick comments:
In `@internal/modelregistry/models_test.go`:
- Around line 54-127: Add two regression tests: (1)
TestModelEntryRejectsProviderNotInAPIProviders — create a model from
validModelEntry(), set ModelEntry.Provider to a provider value not present in
ModelEntry.APIProviders, call model.Validate() and assert it returns an error
that mentions the provider/API providers contract (reference
ModelEntry.Provider, ModelEntry.APIProviders, and ModelEntry.Validate). (2)
TestRegistryDetectsDuplicateNormalizedLookupKeys — create two ModelEntry
instances that differ by ID/API model/alias but normalize to the same lookup key
(e.g., case variants or alias vs API model), try to build the registry with
NewRegistry([]ModelEntry{...}) or insert both into Registry, and assert the
registry creation/insertion fails or reports a duplicate normalized key
(reference NewRegistry, Registry.Get and the normalized lookup behavior) so
duplicate lookup keys are detected in tests.
In `@internal/zeroruntime/provider_test.go`:
- Around line 179-206: Add a new unit test that exercises CollectStream with a
mixture of alias-shaped and normalized TokenUsage events to ensure aggregation
is correct; construct one StreamEvent using a raw/alias usage shape (e.g.,
fields that map to EffectiveInput/Output via aliases) and another StreamEvent
using NormalizeUsage(...) output, send both through the events channel followed
by StreamEventDone, call CollectStream(context.Background(), events) and assert
EffectiveInputTokens(), EffectiveOutputTokens(), ReasoningTokens and TotalTokens
match the expected combined totals; reference CollectStream, StreamEvent,
TokenUsage and NormalizeUsage to locate and implement the test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13b4b4d0-1876-4998-9ba8-a007875d2bff
📒 Files selected for processing (8)
internal/config/contracts.gointernal/config/contracts_test.gointernal/modelregistry/models.gointernal/modelregistry/models_test.gointernal/zeroruntime/helpers.gointernal/zeroruntime/provider_test.gointernal/zeroruntime/types.gointernal/zeroruntime/usage.go
Collaborator
|
Blockers
Non-Blocking
Looks Good
|
Vasanthdev2004
requested changes
Jun 4, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes because NewRegistry() currently registers model entries without calling ModelEntry.Validate(), so invalid model contracts can enter the registry path future provider resolution will depend on. Full details and validation are in my review comment.
Collaborator
Author
|
@Vasanthdev2004 @anandh8x pushed What changed:
Validation run:
Ready for re-review. |
Collaborator
|
Blockers Non-Blocking
Looks Good
|
Vasanthdev2004
approved these changes
Jun 4, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Approved current head ae0f282 after verifying NewRegistry() now validates entries before registering lookup keys and CI/local validation passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Commits
Validation
Summary by CodeRabbit
New Features
Tests