fix(cp): credential rolebinding and project delete#1203
Merged
markturansky merged 6 commits intoalphafrom Apr 3, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds role database seeding via a new migration in the credentials plugin, registers it during initialization, tightens authorization checks for session message access, significantly expands CLI documentation with new commands and workflows, and updates test infrastructure with additional plugin imports. Changes
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
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 (2)
components/ambient-api-server/plugins/sessions/grpc_handler.go (1)
289-299:⚠️ Potential issue | 🔴 CriticalBlock non-service callers when username is empty.
At Line 291, authorization only runs when
username != "". A non-service caller with empty username currently skips ownership checks and can subscribe to arbitrary session streams.Proposed fix
if !middleware.IsServiceCaller(ctx) { username := auth.GetUsernameFromContext(ctx) - if username != "" && (h.grpcServiceAccount == "" || username != h.grpcServiceAccount) { + if username == "" { + return status.Error(codes.PermissionDenied, "not authorized to watch this session") + } + if h.grpcServiceAccount == "" || username != h.grpcServiceAccount { session, svcErr := h.service.Get(ctx, req.GetSessionId()) if svcErr != nil { return grpcutil.ServiceErrorToGRPC(svcErr) } if session.CreatedByUserId == nil || *session.CreatedByUserId != username { return status.Error(codes.PermissionDenied, "not authorized to watch this session") } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ambient-api-server/plugins/sessions/grpc_handler.go` around lines 289 - 299, Authorization currently only runs when username != "", allowing anonymous non-service callers; update the check inside the middleware.IsServiceCaller(ctx) block to treat empty username as unauthorized: after obtaining username via auth.GetUsernameFromContext(ctx) return a PermissionDenied if username == "" (for non-service callers), and then proceed to call h.service.Get(req.GetSessionId()) and verify session.CreatedByUserId matches username when h.grpcServiceAccount is empty or username != h.grpcServiceAccount; reference symbols: middleware.IsServiceCaller, auth.GetUsernameFromContext, h.grpcServiceAccount, h.service.Get, session.CreatedByUserId.components/ambient-cli/cmd/acpctl/apply/cmd.go (1)
290-318:⚠️ Potential issue | 🟠 Major
providerupdates are still dropped for existing credentials.
strategicMerge()now carriesproviderthrough overlays, butbuildCredentialPatch()never compares or sets it. A manifest that only changesproviderwill still come back asunchangedand never persist the update.Also applies to: 652-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ambient-cli/cmd/acpctl/apply/cmd.go` around lines 290 - 318, buildCredentialPatch() fails to detect or set changes to the credential's provider field so provider-only updates are dropped; update the function to compare doc.Provider (after env expansion if applicable) against existing.Provider and, when different and non-empty, call the credential patch builder's Provider(...) method and mark changed = true; apply the same compare-and-set logic to the other duplicated credential-patch helper mentioned (the occurrence covering the 652-663 range) so provider changes persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-api-server/plugins/credentials/migration.go`:
- Around line 67-96: The credentials plugin currently calls
db.RegisterMigration(rolesMigration()) during its import, which runs before the
roles table-creation migration (ID "202603100137") is registered and causes
"roles" table missing; stop registering the migration at import time and instead
export rolesMigration() (leave the function name rolesMigration) and register it
only after the roles table-creation migration is registered (e.g., from main.go
or the roles plugin) by calling db.RegisterMigration(rolesMigration()) after the
roles plugin registers its migration with ID "202603100137". This ensures the
roles table exists before the credentials migration runs.
In `@components/ambient-cli/cmd/acpctl/apply/cmd.go`:
- Around line 305-307: The apply logic currently compares the provided token
variable to existing.Token from client.Credentials().Get(...), but
existing.Token is always empty because PresentCredential() omits the token (only
PresentCredentialToken() / GET /{id}/token returns it), so patch.Token(token) is
always applied; fix by fetching the current token from the separate token
endpoint (the same route/function that uses PresentCredentialToken, e.g., call
the credentials token GET handler via the SDK/client method that returns the
credential token) and compare that returned token to the provided token before
calling patch.Token(token); alternatively, if fetching the current token is not
feasible, change the logic to skip comparing against existing.Token and
unconditionally apply patch.Token(token) when a token is provided (update the
token comparison around token, existing.Token, and patch.Token(token)).
In `@components/ambient-control-plane/internal/informer/informer.go`:
- Around line 163-174: retryLoop is processing retries in strict FIFO from
inf.retryCh so a long-delay entry (re.fireAt far in future) blocks handling of
nearer-term retries and can also block producers; change the scheduling so you
never block the loop on a single long wait: when pulling re := <-inf.retryCh, if
re.fireAt is in the future push it into a min-heap or priority queue keyed by
fireAt and continue the loop, and use a single timer that wakes when the
earliest heap item is due and then pop/dispatch via inf.dispatchHandler;
alternatively spawn a per-retry timer/goroutine (but prefer a heap + timer in
retryLoop) to ensure short-delay retries are dispatched promptly and inf.retryCh
is never blocked by long sleeps.
In `@credentials-testing.md`:
- Around line 18-31: Remove all live environment identifiers and PII from
credentials-testing.md: replace the local secret path
(`~/projects/secrets/github.ambient-pat.token`), the Cluster/Namespace values
(`OSD api.dev-osd-east-1...`, `ambient-code--ambient-s0`), the API server URL,
the acpctl login example URL, and the Active user email (`mturansk@redhat.com`)
with redacted placeholders or generic examples (e.g., <REDACTED_TOKEN_PATH>,
<CLUSTER_URL>, <NAMESPACE>, <API_SERVER_URL>, <LOGIN_CMD>, <ACTIVE_USER>) so no
real endpoints, namespaces, file paths, or personal emails remain in the
committed file; ensure placeholders are clearly named and do not contain any
sensitive fragments.
---
Outside diff comments:
In `@components/ambient-api-server/plugins/sessions/grpc_handler.go`:
- Around line 289-299: Authorization currently only runs when username != "",
allowing anonymous non-service callers; update the check inside the
middleware.IsServiceCaller(ctx) block to treat empty username as unauthorized:
after obtaining username via auth.GetUsernameFromContext(ctx) return a
PermissionDenied if username == "" (for non-service callers), and then proceed
to call h.service.Get(req.GetSessionId()) and verify session.CreatedByUserId
matches username when h.grpcServiceAccount is empty or username !=
h.grpcServiceAccount; reference symbols: middleware.IsServiceCaller,
auth.GetUsernameFromContext, h.grpcServiceAccount, h.service.Get,
session.CreatedByUserId.
In `@components/ambient-cli/cmd/acpctl/apply/cmd.go`:
- Around line 290-318: buildCredentialPatch() fails to detect or set changes to
the credential's provider field so provider-only updates are dropped; update the
function to compare doc.Provider (after env expansion if applicable) against
existing.Provider and, when different and non-empty, call the credential patch
builder's Provider(...) method and mark changed = true; apply the same
compare-and-set logic to the other duplicated credential-patch helper mentioned
(the occurrence covering the 652-663 range) so provider changes persist.
🪄 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
Run ID: 60cf1694-3172-4aca-94a2-86f9799171dd
📒 Files selected for processing (13)
components/ambient-api-server/plugins/credentials/migration.gocomponents/ambient-api-server/plugins/credentials/plugin.gocomponents/ambient-api-server/plugins/sessions/grpc_handler.gocomponents/ambient-api-server/plugins/sessions/plugin.gocomponents/ambient-cli/README.mdcomponents/ambient-cli/cmd/acpctl/apply/cmd.gocomponents/ambient-cli/credential.yamlcomponents/ambient-cli/demo-github.shcomponents/ambient-control-plane/cmd/ambient-control-plane/main.gocomponents/ambient-control-plane/internal/informer/informer.gocomponents/ambient-control-plane/internal/kubeclient/kubeclient.gocomponents/ambient-control-plane/internal/reconciler/kube_reconciler.gocredentials-testing.md
Comment on lines
+67
to
+96
| return &gormigrate.Migration{ | ||
| ID: "202603311216", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| for _, r := range seed { | ||
| permsJSON, err := json.Marshal(r.permissions) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| row := roleRow{ | ||
| ID: api.NewID(), | ||
| Name: r.name, | ||
| DisplayName: r.displayName, | ||
| Description: r.description, | ||
| Permissions: string(permsJSON), | ||
| BuiltIn: true, | ||
| } | ||
| if err := tx.Table("roles"). | ||
| Where("name = ?", r.name). | ||
| FirstOrCreate(&row).Error; err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| names := make([]string, len(seed)) | ||
| for i, r := range seed { | ||
| names[i] = r.name | ||
| } | ||
| return tx.Table("roles").Where("name IN ?", names).Delete(&roleRow{}).Error |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Migration registration / execution code ==="
rg -n -C4 --type=go 'func RegisterMigration|RegisterMigration\(|gormigrate\.New|\.Migrate\('
echo
echo "=== Roles table creation vs credential-role seeding ==="
rg -n -C3 --type=go 'AutoMigrate\(&Role|rolesMigration\(|tx\.Table\("roles"\)' components/ambient-api-server/pluginsRepository: ambient-code/platform
Length of output: 10184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find RegisterMigration definition
echo "=== RegisterMigration function definition ==="
rg -n -B2 -A10 'func RegisterMigration' components/ambient-api-server
echo
echo "=== Migration execution (gormigrate.New and Run/Migrate) ==="
rg -n -B3 -A8 'gormigrate\.New|\.Migrate\(' components/ambient-api-server --type=go | head -100Repository: ambient-code/platform
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where db.RegisterMigration is defined - search in the whole codebase
echo "=== Searching for RegisterMigration in all Go files ==="
rg -n 'RegisterMigration' --type=go | grep -E 'func.*RegisterMigration|type.*RegisterMigration' | head -20
echo
echo "=== Looking for migration registration in server/database setup ==="
fd --type f -name '*.go' | xargs rg -l 'gormigrate.New\|Migrator\|RegisterMigration' | head -10
echo
echo "=== Search for where migrations are collected and executed ==="
rg -n -B5 -A10 'gormigrate\.New' --type=goRepository: ambient-code/platform
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's find where the db variable is defined and RegisterMigration is used
echo "=== Find files that call RegisterMigration ==="
rg -l 'RegisterMigration' --type=go
echo
echo "=== Let's look at the package structure for db ==="
fd --type f -name '*.go' | xargs grep -l 'package.*db\|db\.RegisterMigration' | head -10
echo
echo "=== Check the credentials plugin file more closely ==="
cat -n components/ambient-api-server/plugins/credentials/plugin.go | head -100
echo
echo "=== Look for db import or interface definition ==="
rg -n 'import\|db\.' components/ambient-api-server/plugins/credentials/plugin.go | head -30Repository: ambient-code/platform
Length of output: 929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Credentials plugin file structure ==="
head -50 components/ambient-api-server/plugins/credentials/plugin.go
echo
echo "=== Look for db import/type in credentials plugin ==="
grep -n 'db\.' components/ambient-api-server/plugins/credentials/plugin.go | head -10
echo
echo "=== Check the main init/startup file ==="
fd --type f -name 'main.go' -o -name 'init.go' | grep -v vendor | head -5
echo
echo "=== Look for database initialization code ==="
rg -n 'gormigrate|Migrate\(' --type=go | grep -v test | head -20Repository: ambient-code/platform
Length of output: 2322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find db package in the rh-trex-ai repo ==="
find . -type d -name db | grep -v vendor | grep -v node_modules
echo
echo "=== Look for RegisterMigration in db package ==="
rg -n 'func RegisterMigration\|RegisterMigration\s*=' --type=go | head -20
echo
echo "=== Search in common parent directory for db/migration code ==="
find . -type f -path '*/pkg/db*' -name '*.go' | head -20Repository: ambient-code/platform
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List top-level directories ==="
ls -la
echo
echo "=== Find all Go files that contain RegisterMigration ==="
rg -l 'RegisterMigration' --type=go
echo
echo "=== Search more broadly for where migrations might be run ==="
rg 'RegisterMigration|gormigrate' --type=go -B2 -A2 | head -100Repository: ambient-code/platform
Length of output: 9466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for RegisterMigration definition in pkg/db or similar ==="
find components -type f -name '*.go' -exec grep -l 'RegisterMigration' {} \; | head -5
echo
echo "=== Check if there's a local db package ==="
find components -type d -name 'db'
echo
echo "=== Look for where migrations are actually executed/migrated ==="
rg 'Migrate\(\)|gormigrate\.New' --type=go -B5 -A5 | head -150
echo
echo "=== Search for any migration manager or collector ==="
rg 'migrations.*append|migrations.*\[\]' --type=go | head -20Repository: ambient-code/platform
Length of output: 699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for gormigrate usage and migration storage ==="
rg 'var.*migrations|migrations\s*:=|migrations.*append' --type=go
echo
echo "=== Search for where migrations slice/list is created or sorted ==="
rg 'sort.*migrations|migrations.*sort' --type=go
echo
echo "=== Look for go-gormigrate imports and usage pattern ==="
rg 'gormigrate' --type=go -B2 -A2 | grep -E 'New|migration|register' | head -40
echo
echo "=== Check for migration ID comparison/ordering ==="
rg 'Migration.*ID|\.ID.*Migration' --type=go -B1 -A1 | head -30Repository: ambient-code/platform
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the environments package for db setup ==="
find components -type f -name '*.go' -path '*environments*' | head -10
echo
echo "=== Look for any files that reference Migration or Migrator ==="
rg 'Migration\|Migrator' --type=go -l | head -15
echo
echo "=== Find the actual migration IDs to check ordering ==="
rg 'ID:.*202' --type=go | sort
echo
echo "=== Look at roles migration ID vs credentials rolesMigration ID ==="
rg 'ID.*202603' --type=goRepository: ambient-code/platform
Length of output: 678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check environments/types.go for migration setup ==="
cat components/ambient-api-server/cmd/ambient-api-server/environments/types.go | head -100
echo
echo "=== Check one of the environment files ==="
head -150 components/ambient-api-server/cmd/ambient-api-server/environments/e_development.go
echo
echo "=== Get migration IDs to check order ==="
rg 'ID:\s*"202' --type=goRepository: ambient-code/platform
Length of output: 3591
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify the role migration ID and credentials rolesMigration ID ==="
echo "Roles table creation ID:"
rg 'ID:.*202603100137' --type=go
echo
echo "Credentials rolesMigration ID:"
rg 'ID:.*202603311216' --type=go
echo
echo "=== Search for where migrations are collected and run ==="
rg 'migrations\s*\[\]|migrations.*=.*\[\]|append.*migration' --type=go
echo
echo "=== Check if there's a sort or order function for migrations ==="
rg 'sort\.|SortBy|Less|Swap' --type=go | grep -i migrat
echo
echo "=== Look in pkg/db for migration registration ==="
find . -type f -path '*/pkg/db*' -name '*.go' 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List the plugin init order ==="
git ls-files 'components/ambient-api-server/plugins/*/plugin.go' | sort
echo
echo "=== Check the main server startup file ==="
find components/ambient-api-server -name 'main.go' -type f
echo
echo "=== Look for how plugins are initialized ==="
rg 'plugin\.|import.*plugin' --type=go | grep -v test | head -20
echo
echo "=== Search for where db is initialized and migrations run ==="
rg 'db\.|Database' --type=go components/ambient-api-server/cmd/ambient-api-server/ | head -30Repository: ambient-code/platform
Length of output: 2902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check main.go ==="
head -100 components/ambient-api-server/cmd/ambient-api-server/main.go
echo
echo "=== Look for where db.RegisterMigration is implemented ==="
rg 'RegisterMigration' --type=go -B5 -A10 | grep -v 'plugin.go:' | head -100Repository: ambient-code/platform
Length of output: 6586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for where migrations are collected/stored globally ==="
rg 'var.*migrations|migrations.*append|migrationsRegistry' --type=go
echo
echo "=== Look for gormigrate.New calls and how migrations are passed ==="
rg 'gormigrate\.New|SetMigrations|migration' --type=go | grep -v test | grep -v '\.go-' | head -50
echo
echo "=== Check if db package has RegisterMigration locally ==="
find . -type f -path '*/pkg/db/*' -name '*.go' 2>/dev/null | xargs grep -l 'RegisterMigration' 2>/dev/null || echo "Not found locally"
echo
echo "=== Look in go.mod to understand upstream package ==="
grep 'rh-trex-ai' go.mod || echo "Check if exists"Repository: ambient-code/platform
Length of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check go.mod for the upstream db package version ==="
cat go.mod | head -50
echo
echo "=== Look for any migration execution or migrate command setup ==="
rg 'MigrateCommand|Migrate\(\)|gormigrate\.New' --type=go
echo
echo "=== Check pkg/cmd for migrate command ==="
rg 'MigrateCommand|migrate' --type=go | head -40Repository: ambient-code/platform
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo progfail
echo "=== Find all go.mod files ==="
find . -name 'go.mod' -type f | head -10
echo
echo "=== Check the import structure in the ambient-api-server ==="
find components/ambient-api-server -name 'go.mod' -type f
echo
echo "=== Look for db.RegisterMigration calls and trace back where it's defined ==="
rg 'db\.RegisterMigration\(' --type=go -B10 | head -100
echo
echo "=== Check if db is imported and from where ==="
rg 'import.*".*db"' components/ambient-api-server/plugins/credentials/plugin.go -A5Repository: ambient-code/platform
Length of output: 118
Credentials plugin's rolesMigration depends on roles table but registers before roles table migration in import sequence.
The credentials plugin (imported before roles plugin in main.go) calls db.RegisterMigration(rolesMigration()) which writes to tx.Table("roles"). The roles plugin's table-creation migration has ID 202603100137 while the credentials rolesMigration has ID 202603311216. If migrations execute in plugin-registration/import order rather than by global ID sort, a fresh bootstrap will fail with "table 'roles' doesn't exist" since credentials plugin initializes before roles plugin.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/plugins/credentials/migration.go` around lines
67 - 96, The credentials plugin currently calls
db.RegisterMigration(rolesMigration()) during its import, which runs before the
roles table-creation migration (ID "202603100137") is registered and causes
"roles" table missing; stop registering the migration at import time and instead
export rolesMigration() (leave the function name rolesMigration) and register it
only after the roles table-creation migration is registered (e.g., from main.go
or the roles plugin) by calling db.RegisterMigration(rolesMigration()) after the
roles plugin registers its migration with ID "202603100137". This ensures the
roles table exists before the credentials migration runs.
Comment on lines
305
to
307
| token := os.ExpandEnv(doc.Token) | ||
| if token != "" { | ||
| if token != "" && token != existing.Token { | ||
| patch = patch.Token(token) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Server-side credential endpoints / payloads ==="
rg -n -C3 --type=go 'GetToken|json:"token"|type Credential struct|HandleFunc\("\{id\}/token"|HandleFunc\("/{id}/token"' components/ambient-api-server/plugins/credentials
echo
echo "=== SDK-side credential GET implementation ==="
rg -n -C3 --type=go 'func .*Credentials.*Get\(|func .*GetToken\(|"/credentials"|json:"token"|type Credential struct' components/ambient-sdk/go-sdkRepository: ambient-code/platform
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Server-side: Credential struct definition ==="
rg -n "type Credential struct" --type=go components/ambient-api-server/
echo
echo "=== Server-side: GetToken endpoint/method ==="
rg -n "GetToken" --type=go components/ambient-api-server/plugins/credentials
echo
echo "=== Server-side: Token field in structs ==="
rg -n "json:\"token\"" --type=go components/ambient-api-server/
echo
echo "=== SDK-side: Credentials Get method ==="
rg -n "func.*Get\(" --type=go components/ambient-sdk/go-sdk | head -20
echo
echo "=== SDK-side: Credential struct ==="
rg -n "type Credential" --type=go components/ambient-sdk/go-sdk
echo
echo "=== SDK-side: Token field ==="
rg -n "json:\"token\"" --type=go components/ambient-sdk/go-sdkRepository: ambient-code/platform
Length of output: 3337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Server-side Credential struct (full definition) ==="
sed -n '8,20p' components/ambient-api-server/plugins/credentials/model.go
echo
echo "=== SDK-side Credential struct (full definition) ==="
sed -n '8,20p' components/ambient-sdk/go-sdk/types/credential.go
echo
echo "=== Server-side credential handler Get method ==="
rg -n "func.*Get\(" --type=go -A 15 components/ambient-api-server/plugins/credentials/handler.go | head -40
echo
echo "=== Check if Credential struct fields differ by endpoint ==="
rg -n "CredentialTokenResponse\|type Credential struct" --type=go components/ambient-api-server/plugins/credentials/
echo
echo "=== Server-side handler Get implementation details ==="
rg -n "credentialHandler\) Get\(" --type=go -A 20 components/ambient-api-server/plugins/credentials/handler.goRepository: ambient-code/platform
Length of output: 1513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== PresentCredential function ==="
rg -n "func.*PresentCredential" --type=go -A 20 components/ambient-api-server/plugins/credentials/
echo
echo "=== Check for Token field filtering/redaction ==="
rg -n "Token.*nil\|Token.*=\s*nil" --type=go components/ambient-api-server/plugins/credentials/Repository: ambient-code/platform
Length of output: 2671
Token is never populated from credential reads—only the separate GET /{id}/token endpoint returns it.
The normal Get handler uses PresentCredential() which explicitly omits the Token field from responses (see presenter.go). Only the PresentCredentialToken() function includes it, used solely by the GET /{id}/token endpoint. This means existing.Token from client.Credentials().Get(...) will always be empty, so the condition token != "" && token != existing.Token will always be true if a token is provided, causing the patch to apply on every run and breaking idempotency.
The apply logic needs to either:
- Call the separate token endpoint to fetch the current token for comparison, or
- Omit the token comparison and always patch if a token is provided
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-cli/cmd/acpctl/apply/cmd.go` around lines 305 - 307, The
apply logic currently compares the provided token variable to existing.Token
from client.Credentials().Get(...), but existing.Token is always empty because
PresentCredential() omits the token (only PresentCredentialToken() / GET
/{id}/token returns it), so patch.Token(token) is always applied; fix by
fetching the current token from the separate token endpoint (the same
route/function that uses PresentCredentialToken, e.g., call the credentials
token GET handler via the SDK/client method that returns the credential token)
and compare that returned token to the provided token before calling
patch.Token(token); alternatively, if fetching the current token is not
feasible, change the logic to skip comparing against existing.Token and
unconditionally apply patch.Token(token) when a token is provided (update the
token comparison around token, existing.Token, and patch.Token(token)).
Comment on lines
163
to
+174
| case re := <-inf.retryCh: | ||
| wait := time.Until(re.fireAt) | ||
| if wait > 0 { | ||
| timer := time.NewTimer(wait) | ||
| select { | ||
| case <-time.After(wait): | ||
| case <-timer.C: | ||
| case <-ctx.Done(): | ||
| timer.Stop() | ||
| return | ||
| } | ||
| } | ||
| inf.dispatchEvent(ctx, re.event, re.attempt) | ||
| inf.dispatchHandler(ctx, re.event, re.handlerIndex, re.attempt) |
Contributor
There was a problem hiding this comment.
Retry scheduling is FIFO, so ready retries can be blocked behind long-delay entries.
The single retryLoop sleeps on the first dequeued item. If a fireAt=+30s retry is read before a later fireAt=+2s retry, the shorter retry waits the full 30s. Under a few concurrent failures, that backlog also stops draining retryCh, and Line 220 can block producers entirely.
Also applies to: 204-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-control-plane/internal/informer/informer.go` around lines
163 - 174, retryLoop is processing retries in strict FIFO from inf.retryCh so a
long-delay entry (re.fireAt far in future) blocks handling of nearer-term
retries and can also block producers; change the scheduling so you never block
the loop on a single long wait: when pulling re := <-inf.retryCh, if re.fireAt
is in the future push it into a min-heap or priority queue keyed by fireAt and
continue the loop, and use a single timer that wakes when the earliest heap item
is due and then pop/dispatch via inf.dispatchHandler; alternatively spawn a
per-retry timer/goroutine (but prefer a heap + timer in retryLoop) to ensure
short-delay retries are dispatched promptly and inf.retryCh is never blocked by
long sleeps.
…roles via migration Adds a second migration (202603311216) to the credentials plugin that inserts the two credential-scoped built-in roles into the roles table. Runs after the credentials table migration (202603311215) and after the roles table migration (202603100137), satisfying the ordering dependency without requiring cross-plugin imports. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Non-service callers with an empty username previously skipped ownership checks, allowing unauthenticated access to arbitrary session streams. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/cp-credential-rolebinding-and-project-delete
branch
from
dcdb408 to
90ded74
Compare
… roles table exists rolesMigration() inserts into the roles table; without importing plugins/roles the migration runs against a missing table, causing integration tests to fail. Mirror the full plugin import set from main.go. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-cli/README.md`:
- Line 30: The README's RH SSO example uses a likely-removed flag (--url) in the
command for acpctl login; update the example to use the positional api-url form
used elsewhere (replace the --url flag usage with the <api-url> positional
argument) so the acpctl login example matches the updated flow and will work for
users; ensure the command string referencing acpctl login and the old --url flag
is replaced and any surrounding text reflects the positional <api-url> usage.
- Around line 140-142: The README example for the CLI command "acpctl agent
start" mixes the --project-id flag with a <project-name> placeholder; update the
example so the flag and placeholder match by replacing "<project-name>" with
"<project-id>" (or alternatively change the flag to --project-name if the CLI
actually accepts names) so the sample command reads e.g. acpctl agent start
<agent-name> --project-id <project-id> --prompt "Open a test issue in org/repo";
ensure the change is made in the snippet containing the "acpctl agent start"
example and keep the --project-id flag consistent with the CLI's expected
parameter name.
🪄 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
Run ID: a23c7292-e7fd-49fb-a394-9861ee90fcdb
📒 Files selected for processing (5)
components/ambient-api-server/plugins/credentials/migration.gocomponents/ambient-api-server/plugins/credentials/plugin.gocomponents/ambient-api-server/plugins/credentials/testmain_test.gocomponents/ambient-api-server/plugins/sessions/grpc_handler.gocomponents/ambient-cli/README.md
✅ Files skipped from review due to trivial changes (1)
- components/ambient-api-server/plugins/credentials/testmain_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- components/ambient-api-server/plugins/credentials/plugin.go
- components/ambient-api-server/plugins/sessions/grpc_handler.go
- components/ambient-api-server/plugins/credentials/migration.go
| acpctl login <api-url> --token <your-token> --insecure-skip-tls-verify | ||
|
|
||
| # Use RH SSO | ||
| acpctl login --use-auth-code --url https://ambient-api-server-ambient-code--ambient-s0.apps.int.spoke.dev.us-east-1.aws.paas.redhat.com |
Contributor
There was a problem hiding this comment.
login example uses a likely deprecated flag and may fail.
Line 30 uses --url, but the updated flow in this README uses positional <api-url> (Line 24). If --url was removed, this RH SSO example is broken for users.
Proposed fix
-acpctl login --use-auth-code --url https://ambient-api-server-ambient-code--ambient-s0.apps.int.spoke.dev.us-east-1.aws.paas.redhat.com
+acpctl login https://ambient-api-server-ambient-code--ambient-s0.apps.int.spoke.dev.us-east-1.aws.paas.redhat.com --use-auth-codeAs per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
📝 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
| acpctl login --use-auth-code --url https://ambient-api-server-ambient-code--ambient-s0.apps.int.spoke.dev.us-east-1.aws.paas.redhat.com | |
| acpctl login https://ambient-api-server-ambient-code--ambient-s0.apps.int.spoke.dev.us-east-1.aws.paas.redhat.com --use-auth-code |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-cli/README.md` at line 30, The README's RH SSO example
uses a likely-removed flag (--url) in the command for acpctl login; update the
example to use the positional api-url form used elsewhere (replace the --url
flag usage with the <api-url> positional argument) so the acpctl login example
matches the updated flow and will work for users; ensure the command string
referencing acpctl login and the old --url flag is replaced and any surrounding
text reflects the positional <api-url> usage.
Comment on lines
+140
to
+142
| acpctl agent start <agent-name> \ | ||
| --project-id <project-name> \ | ||
| --prompt "Open a test issue in org/repo" |
Contributor
There was a problem hiding this comment.
Agent start example mixes project-id flag with a project-name placeholder.
Line 141 uses --project-id <project-name>. This is a functional mismatch and can cause failed requests if users pass a name where an ID is required.
Proposed fix
-acpctl agent start <agent-name> \
- --project-id <project-name> \
+acpctl agent start <agent-name> \
+ --project-id <project-id> \
--prompt "Open a test issue in org/repo"As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
📝 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
| acpctl agent start <agent-name> \ | |
| --project-id <project-name> \ | |
| --prompt "Open a test issue in org/repo" | |
| acpctl agent start <agent-name> \ | |
| --project-id <project-id> \ | |
| --prompt "Open a test issue in org/repo" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-cli/README.md` around lines 140 - 142, The README example
for the CLI command "acpctl agent start" mixes the --project-id flag with a
<project-name> placeholder; update the example so the flag and placeholder match
by replacing "<project-name>" with "<project-id>" (or alternatively change the
flag to --project-name if the CLI actually accepts names) so the sample command
reads e.g. acpctl agent start <agent-name> --project-id <project-id> --prompt
"Open a test issue in org/repo"; ensure the change is made in the snippet
containing the "acpctl agent start" example and keep the --project-id flag
consistent with the CLI's expected parameter name.
2 tasks
markturansky
added a commit
that referenced
this pull request
Apr 4, 2026
## Summary
- Seeds `credential:token-reader` and `credential:reader` roles via
migration `202603311216`
- Mounts runner BOT_TOKEN as file with 10-min background refresh loop in
control-plane
- Authorizes runner OIDC service account in `WatchSessionMessages`
- Adds exponential backoff retry in informer error handler
- Adds `acpctl apply -f` credential manifest support and role-binding
commands to CLI
## Test plan
- [ ] Deploy to OSD `ambient-s0` via ArgoCD (gitops MR \!94 already
merged)
- [ ] Verify `credential:token-reader` and `credential:reader` appear in
`acpctl get roles`
- [ ] Create role binding for `github-agent` in `credential-test`
project
- [ ] Start agent session and confirm runner pod retrieves token via
`GET /credentials/{id}/token`
🤖 Generated with [Claude Code](https://claude.ai/code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
## Release Notes
* **New Features**
* Added role seeding and management capabilities to credentials system.
* Enhanced CLI with updated login, project context, and resource
management commands.
* Introduced declarative manifest application via `acpctl apply`.
* Added agent creation and session messaging commands.
* **Bug Fixes**
* Strengthened authorization validation for session message access.
* **Documentation**
* Expanded CLI reference with comprehensive command examples and usage
patterns.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Ambient Code Bot <bot@ambient-code.local>
Co-authored-by: Claude <noreply@anthropic.com>
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
credential:token-readerandcredential:readerroles via migration202603311216WatchSessionMessagesacpctl apply -fcredential manifest support and role-binding commands to CLITest plan
ambient-s0via ArgoCD (gitops MR !94 already merged)credential:token-readerandcredential:readerappear inacpctl get rolesgithub-agentincredential-testprojectGET /credentials/{id}/token🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
acpctl apply.Bug Fixes
Documentation