feat: add layered runtime profile defaults#2270
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces inherited runtime profile defaults at global and workspace scopes. It adds 10 new API endpoints to manage these non-selectable profile layers, updates the execution context to inject default environment variables and secrets with lowest precedence, implements layered profile resolution with precedence merging, and provides a frontend UI for managing defaults alongside per-profile entries. ChangesInherited Runtime Profile Defaults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/persis/store/profile.go (1)
161-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep inherited profiles active and protected when writing them back.
validateStoredProfilenow admits inherited rows based only on the name. That meansUpdate()can persist_global/_workspace.*profiles withProtected=falseorStatusDisabled, even thoughNewInherited()creates them as protected+active andResolver.resolve()treats disabled profiles as an error.Suggested fix
func validateStoredProfile(p *profile.Profile) error { if err := validateProfileStorageName(p.Name); err != nil { return err } + if profile.IsInheritedStorageName(p.Name) { + if !p.Protected { + return errors.New("profile store: inherited profiles must be protected") + } + if p.Status != profile.StatusActive { + return errors.New("profile store: inherited profiles must be active") + } + } for _, entry := range p.Entries { if err := profile.ValidateKey(entry.Key); err != nil { return err } }🤖 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/persis/store/profile.go` around lines 161 - 170, validateStoredProfile currently accepts inherited profile names without ensuring they remain protected and active, allowing Update() to persist inherited profiles with Protected=false or StatusDisabled; modify validateStoredProfile to detect inherited profiles (e.g., names matching the inherited pattern used by profile.NewInherited such as "_global" or prefix "_workspace.") and either set p.Protected = true and p.Status = profile.StatusActive before returning or return an error if those invariants are not met so Update() cannot persist inherited rows with incorrect flags; reference validateStoredProfile, validateProfileStorageName, profile.NewInherited and Resolver.resolve when implementing the check.
🧹 Nitpick comments (1)
ui/src/pages/profiles/index.tsx (1)
936-944: ⚡ Quick winUse the repo-standard wrapping classes for entry keys.
break-allmakes env/secret names much harder to scan because it can split every character. The repo guideline for tables/lists iswhitespace-normal break-words, which still prevents overflow without destroying readability.As per coding guidelines, "Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout overflow."Suggested tweak
- <code className="break-all text-xs">{entry.key}</code> + <code className="whitespace-normal break-words text-xs"> + {entry.key} + </code>🤖 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 `@ui/src/pages/profiles/index.tsx` around lines 936 - 944, Replace the CSS utility classes on the element rendering entry.key to use the repo-standard wrapping classes instead of break-all: locate the <code> element that renders entry.key (near entryLabel(entry) and the surrounding Badge/Button) and change its className from "break-all text-xs" to "whitespace-normal break-words text-xs" so long env/secret names wrap without breaking every character.Source: Coding guidelines
🤖 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 `@api/v1/api.yaml`:
- Around line 8857-8862: The WorkspaceName schema currently only documents that
"all" and "default" are reserved but does not reject them; update the components
schema named WorkspaceName so values "all" and "default" are disallowed (e.g.
add a negative constraint such as not: { enum: ["all","default"] } or use a
negative-lookahead pattern like pattern: "^(?!all$|default$).+$") so that the
path parameter workspaceName (and all routes referencing WorkspaceName) will
fail validation for those reserved names; keep the rest of the schema
(length/format) intact.
In `@internal/profile/inherited.go`:
- Around line 94-106: IsInheritedStorageName currently accepts any hex suffix
but must only accept names that round-trip via WorkspaceInheritedRef; update
IsInheritedStorageName to, after hex.DecodeString(encoded) succeeds, convert the
decoded bytes to string and call workspace.ValidateName(decodedString) (or
equivalent validation function used by WorkspaceInheritedRef) and return true
only if validation passes; keep the existing check for
inheritedGlobalStorageName and prefix inheritedWorkspaceNamePrefix.
In `@internal/service/frontend/api/v1/profiles.go`:
- Around line 248-264: The handler GetWorkspaceRuntimeProfileDefaults (and the
other five similar handlers) resolves the workspace via
workspaceInheritedRuntimeProfileRef before calling the ACL check
requireWorkspaceConfigWrite, allowing an attacker to distinguish existence by
403 vs 404; fix this by performing the authorization check on the validated
workspace name first (call requireWorkspaceConfigWrite(ctx,
request.WorkspaceName) or equivalent) before invoking
workspaceInheritedRuntimeProfileRef/GetByName, or alternatively normalize error
responses so both branches return the same 404/403 behavior; then remove the
now-duplicated requireWorkspaceConfigWrite calls in the five workspace handlers
to avoid repeating the check.
In `@ui/src/pages/profiles/index.tsx`:
- Around line 371-413: The global/workspace default reads are collapsing fetch
errors into an empty layer because only data/isLoading are used and
targetFromGlobalDefaults/targetFromWorkspaceDefaults treat undefined as
entries:[]; update the code to check and propagate query errors (useQuery's
error or isError) instead of treating failures as "not created": detect errors
from the global and workspace queries and pass that error state into the target
builders (or short-circuit and set globalTarget/workspaceTarget to an explicit
error marker) so the UI can surface fetch failures rather than rendering "No
entries" and enabling add/edit against unknown state; reference useQuery,
data/globalDefaults, workspaceDefaults, error/isError, targetFromGlobalDefaults,
and targetFromWorkspaceDefaults when making the change.
---
Outside diff comments:
In `@internal/persis/store/profile.go`:
- Around line 161-170: validateStoredProfile currently accepts inherited profile
names without ensuring they remain protected and active, allowing Update() to
persist inherited profiles with Protected=false or StatusDisabled; modify
validateStoredProfile to detect inherited profiles (e.g., names matching the
inherited pattern used by profile.NewInherited such as "_global" or prefix
"_workspace.") and either set p.Protected = true and p.Status =
profile.StatusActive before returning or return an error if those invariants are
not met so Update() cannot persist inherited rows with incorrect flags;
reference validateStoredProfile, validateProfileStorageName,
profile.NewInherited and Resolver.resolve when implementing the check.
---
Nitpick comments:
In `@ui/src/pages/profiles/index.tsx`:
- Around line 936-944: Replace the CSS utility classes on the element rendering
entry.key to use the repo-standard wrapping classes instead of break-all: locate
the <code> element that renders entry.key (near entryLabel(entry) and the
surrounding Badge/Button) and change its className from "break-all text-xs" to
"whitespace-normal break-words text-xs" so long env/secret names wrap without
breaking every character.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ff0a814-2952-4250-bf2d-ae5d7872d157
📒 Files selected for processing (19)
api/v1/api.gen.goapi/v1/api.yamlinternal/core/exec/context.gointernal/core/exec/context_test.gointernal/persis/store/profile.gointernal/persis/store/profile_test.gointernal/profile/inherited.gointernal/profile/inherited_test.gointernal/profile/manager.gointernal/profile/resolver.gointernal/profile/store.gointernal/runtime/agent/agent.gointernal/runtime/agent/agent_test.gointernal/runtime/agent/export_test.gointernal/runtime/context.gointernal/service/frontend/api/v1/profiles.gointernal/service/frontend/api/v1/profiles_test.goui/src/api/v1/schema.tsui/src/pages/profiles/index.tsx
Comment on lines
+8857
to
+8862
| - name: workspaceName | ||
| in: path | ||
| required: true | ||
| schema: | ||
| $ref: "#/components/schemas/WorkspaceName" | ||
| - $ref: "#/components/parameters/RemoteNode" |
There was a problem hiding this comment.
Enforce the reserved workspace-name exclusions in the schema.
These new workspace-default routes rely on WorkspaceName, but that schema only describes all and default as reserved; it does not reject them. That means /profiles/_workspaces/default still passes spec validation and can create an inherited-layer name that collides with the special workspace selector semantics used elsewhere.
💡 Suggested fix
WorkspaceName:
type: string
description: "Workspace name. The reserved names all and default are not allowed."
minLength: 1
maxLength: 64
pattern: "^[A-Za-z0-9_-]+$"
+ not:
+ enum:
+ - all
+ - defaultAs per coding guidelines, "Use REST API endpoints defined in OpenAPI spec at api/v1/api.yaml — generate Go server code via oapi-codegen with make api."
Also applies to: 8909-8914, 8970-8974, 9036-9040, 9101-9105
🤖 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 `@api/v1/api.yaml` around lines 8857 - 8862, The WorkspaceName schema currently
only documents that "all" and "default" are reserved but does not reject them;
update the components schema named WorkspaceName so values "all" and "default"
are disallowed (e.g. add a negative constraint such as not: { enum:
["all","default"] } or use a negative-lookahead pattern like pattern:
"^(?!all$|default$).+$") so that the path parameter workspaceName (and all
routes referencing WorkspaceName) will fail validation for those reserved names;
keep the rest of the schema (length/format) intact.
Sources: Coding guidelines, Learnings
Comment on lines
+94
to
+106
| func IsInheritedStorageName(name string) bool { | ||
| if name == inheritedGlobalStorageName { | ||
| return true | ||
| } | ||
| if !strings.HasPrefix(name, inheritedWorkspaceNamePrefix) { | ||
| return false | ||
| } | ||
| encoded := strings.TrimPrefix(name, inheritedWorkspaceNamePrefix) | ||
| if encoded == "" { | ||
| return false | ||
| } | ||
| _, err := hex.DecodeString(encoded) | ||
| return err == nil |
There was a problem hiding this comment.
Reject workspace storage names that cannot round-trip through WorkspaceInheritedRef.
IsInheritedStorageName currently accepts any hex suffix, but WorkspaceInheritedRef only produces names whose decoded suffix also passes workspace.ValidateName. That mismatch lets rows like _workspace.2f be persisted, hidden by List(), and never fetched again through GetInherited(), because callers cannot recreate a matching InheritedRef.
Suggested fix
func IsInheritedStorageName(name string) bool {
if name == inheritedGlobalStorageName {
return true
}
if !strings.HasPrefix(name, inheritedWorkspaceNamePrefix) {
return false
}
encoded := strings.TrimPrefix(name, inheritedWorkspaceNamePrefix)
if encoded == "" {
return false
}
- _, err := hex.DecodeString(encoded)
- return err == nil
+ decoded, err := hex.DecodeString(encoded)
+ if err != nil {
+ return false
+ }
+ return workspace.ValidateName(string(decoded)) == nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func IsInheritedStorageName(name string) bool { | |
| if name == inheritedGlobalStorageName { | |
| return true | |
| } | |
| if !strings.HasPrefix(name, inheritedWorkspaceNamePrefix) { | |
| return false | |
| } | |
| encoded := strings.TrimPrefix(name, inheritedWorkspaceNamePrefix) | |
| if encoded == "" { | |
| return false | |
| } | |
| _, err := hex.DecodeString(encoded) | |
| return err == nil | |
| func IsInheritedStorageName(name string) bool { | |
| if name == inheritedGlobalStorageName { | |
| return true | |
| } | |
| if !strings.HasPrefix(name, inheritedWorkspaceNamePrefix) { | |
| return false | |
| } | |
| encoded := strings.TrimPrefix(name, inheritedWorkspaceNamePrefix) | |
| if encoded == "" { | |
| return false | |
| } | |
| decoded, err := hex.DecodeString(encoded) | |
| if err != nil { | |
| return false | |
| } | |
| return workspace.ValidateName(string(decoded)) == nil | |
| } |
🤖 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/profile/inherited.go` around lines 94 - 106, IsInheritedStorageName
currently accepts any hex suffix but must only accept names that round-trip via
WorkspaceInheritedRef; update IsInheritedStorageName to, after
hex.DecodeString(encoded) succeeds, convert the decoded bytes to string and call
workspace.ValidateName(decodedString) (or equivalent validation function used by
WorkspaceInheritedRef) and return true only if validation passes; keep the
existing check for inheritedGlobalStorageName and prefix
inheritedWorkspaceNamePrefix.
Comment on lines
+248
to
+264
| func (a *API) GetWorkspaceRuntimeProfileDefaults( | ||
| ctx context.Context, | ||
| request api.GetWorkspaceRuntimeProfileDefaultsRequestObject, | ||
| ) (api.GetWorkspaceRuntimeProfileDefaultsResponseObject, error) { | ||
| target, clientErr, err := a.workspaceInheritedRuntimeProfileRef(ctx, request.WorkspaceName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if clientErr != nil { | ||
| if clientErr.Code == api.ErrorCodeNotFound { | ||
| return api.GetWorkspaceRuntimeProfileDefaults404JSONResponse(*clientErr), nil | ||
| } | ||
| return api.GetWorkspaceRuntimeProfileDefaults400JSONResponse(*clientErr), nil | ||
| } | ||
| if err := a.requireWorkspaceConfigWrite(ctx, target.workspaceName); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Authorize before resolving workspace existence.
These workspace-default handlers load the workspace from storage before requireWorkspaceConfigWrite, so an unauthorized caller can distinguish an existing workspace from a missing one by comparing 403 vs 404 responses. Move the ACL check onto the validated workspace name before GetByName, or normalize both branches to the same response.
Suggested direction
func (a *API) workspaceInheritedRuntimeProfileRef(
ctx context.Context,
name string,
) (workspaceInheritedRuntimeProfileTarget, *api.Error, error) {
if a.workspaceStore == nil {
return workspaceInheritedRuntimeProfileTarget{}, nil, workspaceStoreUnavailable()
}
workspaceName, err := validateWorkspaceParam(strings.TrimSpace(name))
if err != nil {
return workspaceInheritedRuntimeProfileTarget{}, serviceAPIError(err), nil
}
if workspaceName == "" {
return workspaceInheritedRuntimeProfileTarget{}, ptrOf(runtimeProfileBadRequest("workspace name is required")), nil
}
+ if err := a.requireWorkspaceConfigWrite(ctx, workspaceName); err != nil {
+ return workspaceInheritedRuntimeProfileTarget{}, nil, err
+ }
ws, err := a.workspaceStore.GetByName(ctx, workspaceName)
if err != nil {
if errors.Is(err, workspace.ErrWorkspaceNotFound) {
return workspaceInheritedRuntimeProfileTarget{}, &api.Error{
Code: api.ErrorCodeNotFound,
Message: "Resource not found",
}, nil
}
return workspaceInheritedRuntimeProfileTarget{}, nil, err
}Then drop the duplicated requireWorkspaceConfigWrite calls from the five workspace handlers.
Also applies to: 274-290, 301-317, 328-344, 355-370, 761-783
🤖 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/service/frontend/api/v1/profiles.go` around lines 248 - 264, The
handler GetWorkspaceRuntimeProfileDefaults (and the other five similar handlers)
resolves the workspace via workspaceInheritedRuntimeProfileRef before calling
the ACL check requireWorkspaceConfigWrite, allowing an attacker to distinguish
existence by 403 vs 404; fix this by performing the authorization check on the
validated workspace name first (call requireWorkspaceConfigWrite(ctx,
request.WorkspaceName) or equivalent) before invoking
workspaceInheritedRuntimeProfileRef/GetByName, or alternatively normalize error
responses so both branches return the same 404/403 behavior; then remove the
now-duplicated requireWorkspaceConfigWrite calls in the five workspace handlers
to avoid repeating the check.
Comment on lines
+371
to
+413
| const { | ||
| data: globalDefaults, | ||
| mutate: mutateGlobalDefaults, | ||
| isLoading: isGlobalDefaultsLoading, | ||
| } = useQuery( | ||
| '/profiles/_global', | ||
| whenEnabled(canManageProtectedProfiles, { | ||
| params: { | ||
| query: { remoteNode }, | ||
| }, | ||
| }) | ||
| ); | ||
| const { | ||
| data: workspaceDefaults, | ||
| mutate: mutateWorkspaceDefaults, | ||
| isLoading: isWorkspaceDefaultsLoading, | ||
| } = useQuery( | ||
| '/profiles/_workspaces/{workspaceName}', | ||
| selectedWorkspaceName && canManageWorkspaceDefaults | ||
| ? { | ||
| params: { | ||
| path: { workspaceName: selectedWorkspaceName }, | ||
| query: { remoteNode }, | ||
| }, | ||
| } | ||
| : null | ||
| ); | ||
| const globalTarget = useMemo( | ||
| () => | ||
| targetFromGlobalDefaults(globalDefaults, canManageProtectedProfiles), | ||
| [canManageProtectedProfiles, globalDefaults] | ||
| ); | ||
| const workspaceTarget = useMemo( | ||
| () => | ||
| selectedWorkspaceName | ||
| ? targetFromWorkspaceDefaults( | ||
| selectedWorkspaceName, | ||
| workspaceDefaults, | ||
| canManageWorkspaceDefaults | ||
| ) | ||
| : null, | ||
| [canManageWorkspaceDefaults, selectedWorkspaceName, workspaceDefaults] | ||
| ); |
There was a problem hiding this comment.
Don't collapse failed default-layer reads into an empty layer.
These queries only use data/isLoading, and the target builders turn undefined into entries: []. A 403/500/network failure will therefore render as “No entries” and expose add/edit actions against an unknown state. Please distinguish the “not created yet” case from real fetch failures and surface the latter in the page state instead of treating them as empty defaults.
🤖 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 `@ui/src/pages/profiles/index.tsx` around lines 371 - 413, The global/workspace
default reads are collapsing fetch errors into an empty layer because only
data/isLoading are used and targetFromGlobalDefaults/targetFromWorkspaceDefaults
treat undefined as entries:[]; update the code to check and propagate query
errors (useQuery's error or isError) instead of treating failures as "not
created": detect errors from the global and workspace queries and pass that
error state into the target builders (or short-circuit and set
globalTarget/workspaceTarget to an explicit error marker) so the UI can surface
fetch failures rather than rendering "No entries" and enabling add/edit against
unknown state; reference useQuery, data/globalDefaults, workspaceDefaults,
error/isError, targetFromGlobalDefaults, and targetFromWorkspaceDefaults when
making the change.
There was a problem hiding this comment.
1 issue found across 20 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
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
1 participant
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
Testing
Summary by cubic
Add global and workspace default runtime profile layers inherited into runs with the lowest precedence. Adds API and UI to manage these defaults, keeps secrets masked, and does not change how users select profiles.
New Features
GET/PATCH /profiles/_global,GET/PATCH /profiles/_workspaces/{name}; variables and secrets subroutes); secret values are never returned.WithDefaultEnvVarsandWithDefaultSecrets.useCanWriteForWorkspace.Bug Fixes
Written for commit 6cb14ab. Summary will update on new commits.
Summary by CodeRabbit
Release Notes