Resolve hierarchical runtime profiles - #62
Conversation
Gavel summary
Totals: 0 passed · 0 failed · 0 skipped · - |
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change replaces request-scoped runtime settings with resolved runtime profiles. It adds layered specification resolution, model restrictions, quota enforcement, profile-aware chat handling, and propagation of resolution data into authoritative execution records. ChangesRuntime profile integration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🧹 Nitpick comments (4)
pkg/aichat/service.go (1)
139-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
handleToolsloads a profile it does not use.The handler discards the profile and only propagates the load error. The intent appears to be a consistent failure mode across chat endpoints when profile resolution is broken. Add a short comment so the call is not removed later as dead work.
🤖 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 `@pkg/aichat/service.go` around lines 139 - 143, In Service.handleTools, add a short comment immediately before the runtimeProfile call explaining that the profile is intentionally loaded only to preserve consistent error handling when profile resolution fails across chat endpoints.pkg/api/spec_layers_ginkgo_test.go (1)
69-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding specs for the rejection paths.
The three specs cover the success paths.
validateSpecLayer,parseOptionalDuration, and the empty-catalog guard inResolveSpecLayerscarry most of the new failure behavior and none of it is exercised. Add cases for an invalid scope, an empty model selector, a quota declared on a surface or user layer, a duplicate quota name within one layer, and a malformedTimeoutstring.🤖 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 `@pkg/api/spec_layers_ginkgo_test.go` around lines 69 - 99, Add rejection-focused specs for ResolveSpecLayers and its validation helpers, covering invalid scope, empty model selector, quotas on surface or user layers, duplicate quota names within one layer, and malformed Budget.Timeout values. Assert each case returns an error and preserve the existing success-path coverage.pkg/aichat/provider_config.go (1)
61-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the two restriction-layer finders.
modelRestrictionLayerandruntimeRestrictionLayerdiffer only in the predicate applied tolayer.Constraints.Models. A single helper that walksresolved.Tracein reverse and takes afunc([]string) boolwould remove the duplicated traversal.🤖 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 `@pkg/aichat/provider_config.go` around lines 61 - 79, Collapse modelRestrictionLayer and runtimeRestrictionLayer into one reverse-traversal helper that accepts a predicate over layer.Constraints.Models. Preserve the existing non-empty-model constraint check and have each caller supply its respective model or runtime predicate, returning the same restricting *api.SpecLayer or nil behavior.pkg/cli/serve_chat.go (1)
40-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the static layer once, outside the per-request closure.
The
captain servelayer is constant for the process. The closure rebuilds and re-resolves it on every chat, models, runtimes, and tools request, and each call sorts, merges, and clones. Resolving once innewCaptainChatServicealso moves a configuration error to startup, where it fails the command instead of returning HTTP 500 per request.♻️ Proposed refactor
+ resolved, err := api.ResolveSpecLayers(api.SpecLayer{ + Name: "captain serve", Scope: api.SpecLayerGlobal, + Spec: api.Spec{ + Model: api.Model{Name: "sol", Mode: registry.ModeAgent}, + Setup: &shell.Setup{Cwd: cwd}, + }, + }) + if err != nil { + return nil, nil, err + } + profile := aichat.RuntimeProfile{ + System: "You are Captain's coding-agent launcher assistant. Use Captain and Clicky tools when useful, " + + "prefer read-only inspection unless the user explicitly asks for edits, and keep follow-up guidance concise.", + Resolved: resolved, + } chat := aichat.NewService(aichat.ServiceOptions{ Profile: aichat.RuntimeProfileProviderFunc(func(context.Context) (aichat.RuntimeProfile, error) { - resolved, err := api.ResolveSpecLayers(api.SpecLayer{ - Name: "captain serve", Scope: api.SpecLayerGlobal, - Spec: api.Spec{ - Model: api.Model{Name: "sol", Mode: registry.ModeAgent}, - Setup: &shell.Setup{Cwd: cwd}, - }, - }) - if err != nil { - return aichat.RuntimeProfile{}, err - } - return aichat.RuntimeProfile{ - System: "You are Captain's coding-agent launcher assistant. Use Captain and Clicky tools when useful, " + - "prefer read-only inspection unless the user explicitly asks for edits, and keep follow-up guidance concise.", - Resolved: resolved, - }, nil + return profile, nil }),This shares one
Resolvedvalue across requests.requestSpeccopiesTracebefore appending the user layer, so no request mutates the shared profile.🤖 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 `@pkg/cli/serve_chat.go` around lines 40 - 56, In newCaptainChatService, construct and resolve the constant “captain serve” SpecLayer once before creating the RuntimeProfileProviderFunc, returning the startup error immediately if resolution fails. Have the per-request closure reuse the shared Resolved value, preserving requestSpec’s Trace copy before adding any user-specific layer and avoiding mutation of the shared profile.
🤖 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 `@pkg/aichat/approval_execution.go`:
- Around line 19-21: The approval resume flow around ResolveToolApproval and
ExecuteStream must revalidate the admitted runtime profile before execution.
Preserve the persisted api.ResolvedSpec from renderedSpecMap and apply it when
resuming, or rerun the complete profile resolution, model-constraint, and quota
checks before using the current ProviderConfig; do not execute solely from the
decoded api.Spec.
In `@pkg/aichat/provider_config.go`:
- Around line 81-93: Update runtimeAllowed to avoid treating a provider-wide
selector prefix as sufficient for every mode sharing that provider. Match
selectors against the runtime mode’s DefaultModel or require the selector to
resolve to the specific backend, ensuring annotateProfileRuntimes only enables
modes with at least one allowed model while preserving validateResolvedModels
enforcement.
In `@pkg/api/spec_layers_ginkgo_test.go`:
- Around line 37-38: Strengthen the immutability checks in the resolution test
by initializing context.Spec.Model.Name and global.Spec.Model.Effort with
non-empty values before calling ResolveSpecLayers, then assert each input layer
still matches its original value afterward instead of asserting emptiness.
In `@pkg/api/spec_layers.go`:
- Around line 177-192: Update intersectModels to trim selector names from both
current and restrictive before comparing or returning them. Apply the same
normalization when current is empty, build allowed from trimmed restrictive
values, and compare trimmed current values so padded selectors intersect
correctly without producing whitespace-padded results.
- Around line 294-299: Update cloneSpecLayer to deep-copy mutable values inside
layer.Spec.CLIArgs, including interface-contained maps and slices such as
[]string, rather than relying on merge.Apply’s shallow map copy. Preserve scalar
values while cloning each nested mutable value so later CLI argument mutations
cannot affect the resolved layer.
---
Nitpick comments:
In `@pkg/aichat/provider_config.go`:
- Around line 61-79: Collapse modelRestrictionLayer and runtimeRestrictionLayer
into one reverse-traversal helper that accepts a predicate over
layer.Constraints.Models. Preserve the existing non-empty-model constraint check
and have each caller supply its respective model or runtime predicate, returning
the same restricting *api.SpecLayer or nil behavior.
In `@pkg/aichat/service.go`:
- Around line 139-143: In Service.handleTools, add a short comment immediately
before the runtimeProfile call explaining that the profile is intentionally
loaded only to preserve consistent error handling when profile resolution fails
across chat endpoints.
In `@pkg/api/spec_layers_ginkgo_test.go`:
- Around line 69-99: Add rejection-focused specs for ResolveSpecLayers and its
validation helpers, covering invalid scope, empty model selector, quotas on
surface or user layers, duplicate quota names within one layer, and malformed
Budget.Timeout values. Assert each case returns an error and preserve the
existing success-path coverage.
In `@pkg/cli/serve_chat.go`:
- Around line 40-56: In newCaptainChatService, construct and resolve the
constant “captain serve” SpecLayer once before creating the
RuntimeProfileProviderFunc, returning the startup error immediately if
resolution fails. Have the per-request closure reuse the shared Resolved value,
preserving requestSpec’s Trace copy before adding any user-specific layer and
avoiding mutation of the shared profile.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31e2a8d9-811f-4313-a8d7-e9f4e864a644
📒 Files selected for processing (14)
pkg/aichat/aimock_lifecycle_integration_test.gopkg/aichat/approval_execution.gopkg/aichat/execution.gopkg/aichat/execution_authority_ginkgo_test.gopkg/aichat/execution_database_authority.gopkg/aichat/messages.gopkg/aichat/provider_config.gopkg/aichat/runtime_profile_ginkgo_test.gopkg/aichat/runtime_settings.gopkg/aichat/service.gopkg/aichat/service_ginkgo_test.gopkg/api/spec_layers.gopkg/api/spec_layers_ginkgo_test.gopkg/cli/serve_chat.go
Gavel summary
Totals: 3555 passed · 0 failed · 10 skipped · 2m41s |
Claude-Session-Id: 019fef59-46c9-7b83-bc02-6bd4246d90de
39c0427 to
07c2c4d
Compare
3866b41 to
07c2c4d
Compare
| type RuntimeLimits struct { | ||
| MaxInputTokens int `json:"maxInputTokens,omitempty" yaml:"maxInputTokens,omitempty"` | ||
| Budget Budget `json:"budget,omitempty" yaml:"budget,omitempty"` | ||
| } | ||
|
|
||
| // RuntimeQuota is one independently enforced usage allowance. | ||
| type RuntimeQuota struct { | ||
| Name string `json:"name" yaml:"name"` | ||
| Scope SpecLayerScope `json:"scope" yaml:"scope"` | ||
| Layer string `json:"layer" yaml:"layer"` | ||
| TokenLimit int `json:"tokenLimit,omitempty" yaml:"tokenLimit,omitempty"` | ||
| TokensUsed int `json:"tokensUsed,omitempty" yaml:"tokensUsed,omitempty"` | ||
| CostLimitUSD float64 `json:"costLimitUsd,omitempty" yaml:"costLimitUsd,omitempty"` | ||
| CostUsedUSD float64 `json:"costUsedUsd,omitempty" yaml:"costUsedUsd,omitempty"` | ||
| } |
There was a problem hiding this comment.
RuntimeLimits means per run while RuntimeQuota means per arbitrary period (monthly for eg). The name feels overloaded.
Perhaps, RunLimits vs (UsageQuota or PeriodQuota)?
|
I don't see a way to set quota ? |
adityathebe
left a comment
adityathebe
left a comment
There was a problem hiding this comment.
Three findings from a code review of this PR — the first two are functional bugs verified by executing the functions on this branch; the third is an error-classification issue. Details inline.
Generated by Claude Code
| return nil | ||
| } | ||
|
|
||
| func runtimeAllowed(models []string, backend api.Backend) bool { |
There was a problem hiding this comment.
runtimeAllowed cannot match bare model-name selectors, so a name-style constraint disables every runtime family.
AllowsModel matches selectors by model name, so a profile constraint like Models: ["gpt-5.6-sol"] is valid and /api/chat/models correctly shows that model as available. But runtimeAllowed only recognizes a selector as covering a backend via an explicit provider prefix (e.g. openai/) or via (api.Model{Name: selector}).Expand() — and Expand() returns an empty Backend for any bare name (no : or ,). Verified on this branch: runtimeAllowed([]string{"gpt-5.6-sol"}, api.BackendOpenAI) == false.
As a result annotateProfileRuntimes marks all runtime modes Disabled — including the backend whose model the profile explicitly allows — while /api/chat/models says the model is available: a self-contradictory catalog.
Suggestion: derive backend coverage from the same selector-matching semantics as AllowsModel/modelSelectorMatches in pkg/api/spec_layers.go (e.g. resolve the selector against the model registry and take the resolved model's backend) instead of re-implementing matching here.
Generated by Claude Code
| } | ||
| out := make([]string, 0, len(current)) | ||
| for _, model := range current { | ||
| if allowed[model] { |
There was a problem hiding this comment.
intersectModels trims only one side, so a whitespace-only difference empties the catalog and hard-fails profile resolution.
The restrictive layer's selectors are inserted trimmed (allowed[strings.TrimSpace(model)] = true), but current — copied verbatim from the first constraining layer via append([]string(nil), restrictive...) — is looked up untrimmed. A global layer with Models: [" gpt-5.4 "] intersected with a context layer's Models: ["gpt-5.4"] therefore produces an empty catalog. Verified on this branch: ResolveSpecLayers fails with spec layer "b" leaves the effective model catalog empty for semantically identical selectors — and with a request-time provider (as wired in serve_chat.go) that rejects every chat request.
validateSpecLayer trims only for its own checks and never normalizes the stored list. Suggestion: normalize selectors once when they enter the resolved constraints (and/or look up with allowed[strings.TrimSpace(model)] here) so both sides of the intersection use the same canonical form.
Generated by Claude Code
| } | ||
| } | ||
| spec, err := requestSpec(chat, settings, attachments) | ||
| resolved, err := requestSpec(chat, profile, attachments) |
There was a problem hiding this comment.
Server-side profile failures from requestSpec surface as HTTP 400.
Every error returned by requestSpec is mapped to http.StatusBadRequest, but some of them are server configuration defects rather than client mistakes: a RuntimeProfileProvider that returns Resolved without a Trace yields "chat runtime profile must include its resolution trace", and internal ResolveSpecLayers failures are wrapped as resolve chat runtime profile: .... Both would be reported as client errors, so monitoring/alerting on 5xx never sees them.
Suggestion: distinguish profile/resolution failures (500) from request-validation failures (400) — e.g. sentinel/typed errors from requestSpec, or resolve the profile separately from per-request validation so each path can map to the right status.
Generated by Claude Code
Bare model selectors could disable every runtime, and whitespace-only selector differences could empty an otherwise valid catalog. Server-owned profile resolution failures were also reported as client errors. Match runtime constraints against concrete registry models, normalize catalog intersections, validate profiles before layering request fields, and clarify per-run limit and accumulated quota names.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes