feat(ambient-ui): Dashboard, agent detail, multi-session sidebar, responsive layout

[jsell-rh]

Summary

Major ambient-ui feature batch: Dashboard landing page, agent detail page with CRUD, enhanced sessions table, keyboard navigation, unified multi-session chat sidebar, and responsive layout.

Dashboard (default landing page)

• Attention banner surfacing failed sessions, needs-review, needs-input
• Active work cards grouped by Jira/PR annotations
• Recent activity feed with last 10 completed sessions
• Sidebar badge count for items needing attention
• Container-query responsive layout for narrow widths

Agent detail page

• Full page at /{projectId}/agents/{agentId} with Manifest + Run History tabs
• Manifest tab: editable config form + live YAML preview + annotations
• Run History: compact table, click opens session in sidebar
• Agent CRUD: create, update, delete with lifecycle badges (Unmanaged/GitOps)
• "Run Test Session" popover creates session and opens in sidebar

Multi-session chat sidebar

• Unified chat sidebar replaces broken test session pane
• Tab strip with phase-colored dots, session names, close-per-tab
• Always-visible tabs even with single session
• "+" button opens agent picker popover for quick session creation
• Test mode toolbar (re-run, save, delete) when session has test annotation
• Collapsed state: vertical dot stack with tooltips
• Cap at 8 concurrent sessions

Sessions table enhancements

• Work Item column (Jira/PR/MR/Gerrit chips)
• Review Status column (colored badges)
• Bulk operations (stop/delete with confirmation)
• Timestamp toggle (relative/absolute)
• Test session "test" badge
• Agent name as clickable link to agent detail
• "Show test runs" toggle (hidden by default)

Other

• Sidebar restructured into Operate/Build nav groups
• Keyboard navigation: j/k row nav, Cmd+K command palette, Escape-back
• Centralized model options matching runner's VERTEX_MODEL_MAP
• Git commit SHA in sidebar footer
• API server fix: agent PATCH now persists all fields (display_name, repo_url, llm_model, description, llm_temperature, llm_max_tokens)

Test plan

• Dashboard renders attention banner for failed/needs-input sessions
• Agent detail page: edit config, save, verify YAML updates
• Run Test Session creates session and opens in sidebar
• Multiple sessions open as tabs in sidebar, switching works
• "+" button lists agents, creates session on click
• Sessions table: Work Item and Review columns render
• Bulk select + stop/delete works
• Test sessions filtered from main Sessions page by default
• Cmd+K opens command palette
• Dashboard responsive when sidebar is open

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Full project-scoped agent CRUD UI (create sheet, detail page with Manifest/Overview/Config/Sessions tabs), agent YAML export/download, test-session flows, command palette, multi-session chat sidebar, dashboard widgets (Attention, Active Work, Recent Activity)

• Improvements

  • Bulk session actions, richer fleet/agents tables (work-item, agent links, review badges, test-run toggle, time-format toggle), keyboard table navigation, session creation accepts annotations, agent metadata and LLM settings are surfaced and patchable

• Chores

  • UI displays short git commit hash from build environment

[netlify]

✅ Deploy Preview for cheerful-kitten-f556a0 canceled.

Name	Link
🔨 Latest commit	634cb43
🔍 Latest deploy log	https://app.netlify.com/projects/cheerful-kitten-f556a0/deploys/6a20d32953a9530008f3325a

[coderabbitai]

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-api-server/plugins/agents/migration.go`:
- Around line 56-57: The DB migration (agentSchemaExpansionMigration) adds
defaults llm_temperature=0.7 and llm_max_tokens=4000 but ConvertAgent leaves
missing API fields as Go zero values and sqlAgentDao.Create/Replace persists
those zeros so the schema defaults are never applied; to fix, set the LLM
defaults in the model conversion path (e.g., in ConvertAgent and/or the agent
BeforeCreate hook) to 0.7 and 4000 when the incoming fields are omitted, update
ignite_handler.go to stop forwarding zero values (it will receive the proper
defaults), and update TestAgentPostLlmDefaults to assert 0.7/4000 instead of
0.0/0. Ensure you reference ConvertAgent, BeforeCreate (or the model
constructor), sqlAgentDao.Create/Replace, agentSchemaExpansionMigration,
ignite_handler.go, and TestAgentPostLlmDefaults when making the changes.

🪄 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: be8bed29-a3b6-4a7a-b33f-f01e9a4b5d7a

📥 Commits

Reviewing files that changed from the base of the PR and between 6b34edd and 4304c73.

📒 Files selected for processing (2)

• components/ambient-api-server/plugins/agents/migration.go
• components/ambient-api-server/plugins/agents/model.go

💤 Files with no reviewable changes (1)

• components/ambient-api-server/plugins/agents/model.go

[markturansky]

Round 3 — one blocking test bug, two minor notes

The sentinel approach is the right call. ConvertAgent sets -1.0 when the caller omits temperature, BeforeCreate converts -1.0 → 0.7, and the PATCH path uses Replace() so BeforeCreate never fires there — meaning PATCH llm_temperature: 0.0 correctly stores 0.0. The TestAgentPatchTemperatureZeroPreserved test verifies exactly this case. Good.

---

[BLOCKING] TestAgentPostLlmDefaults asserts wrong expected values

The test was committed in f40dd08 (2026-06-03T22:08:32Z) and the sentinel fix was committed two hours later in 98aabf4 (2026-06-04T00:28:24Z). The test was written against the broken Round 2 state (no BeforeCreate defaults) and not updated after the fix.

What the test asserts:

Expect(*agentOutput.LlmTemperature).To(BeNumerically("~", 0.0, 0.001))
Expect(*agentOutput.LlmMaxTokens).To(Equal(int32(0)))

What the implementation actually produces:

• LlmTemperature: ConvertAgent sets sentinel -1.0 → BeforeCreate converts to 0.7 → API returns 0.7
• LlmMaxTokens: ConvertAgent leaves at 0 (Go zero) → BeforeCreate <= 0 guard sets to 4000 → API returns 4000

This test will fail. Fix the assertions:

Expect(*agentOutput.LlmTemperature).To(BeNumerically("~", 0.7, 0.001))
Expect(*agentOutput.LlmMaxTokens).To(Equal(int32(4000)))

---

[MINOR] Explicit llm_temperature: -1.0 on POST is silently converted to 0.7

If a client intentionally POSTs llm_temperature: -1.0, ConvertAgent takes the if agent.LlmTemperature != nil branch and sets c.LlmTemperature = -1.0. Since -1.0 == unsetTemperature, BeforeCreate converts it to 0.7 — silently discarding the client's value. -1.0 is out of range for Claude anyway (valid range is 0.0–2.0), so the practical risk is low, but there's no validation rejecting it. Consider adding a range check on create/patch or documenting the reserved value.

---

[MINOR] LlmMaxTokens uses <= 0 guard, not a sentinel

LlmTemperature uses a proper sentinel (-1.0) to distinguish "unset" from "explicit zero". LlmMaxTokens uses if d.LlmMaxTokens <= 0 { d.LlmMaxTokens = 4000 }, which means:

• PATCH llm_max_tokens: 0 → correctly stores 0 (BeforeCreate doesn't run) ✓
• POST llm_max_tokens: 0 → silently becomes 4000 (BeforeCreate does run)

This is an inconsistency — you can set max_tokens to 0 via PATCH but not via POST. 0 max_tokens is non-functional in practice so the real-world impact is minimal, but a symmetric unsetMaxTokens = -1 constant (mirroring the temperature approach) would remove the asymmetry and make the intent explicit.

---

What's solid: migration (Round 2, still correct), sentinel logic, ConvertAgent, PATCH handler, TestAgentPatchTemperatureZeroPreserved, cross-project isolation test. One line fix to TestAgentPostLlmDefaults unblocks this.

[markturansky]

Round 4 — LGTM

All three rounds of findings resolved.

Sentinel approach is correct end-to-end:

• ConvertAgent: nil temperature → unsetTemperature (-1.0), nil max_tokens → unsetMaxTokens (-1)
• BeforeCreate: exact equality checks convert sentinels to defaults (0.7, 4000) — no false positives on explicit zeros
• Patch handler uses Replace(), so BeforeCreate never fires there — PATCH llm_temperature: 0.0 and PATCH llm_max_tokens: 0 both persist correctly
• TestAgentPatchTemperatureZeroPreserved verifies the 0.0 round-trip explicitly

Test suite is now correct:

• TestAgentPostLlmDefaults asserts 0.7/4000 (not 0.0/0)
• 8 integration tests covering GET (auth, 404, 200), POST (all fields + defaults), PATCH (updates + zero preservation), paging, list search, delete, cross-project isolation

Migration (Round 2): DEFAULT values verified correct on all four new columns.

One note carried forward from Round 3: a client that deliberately POSTs llm_temperature: -1.0 or llm_max_tokens: -1 will have those silently converted to defaults, since those values collide with the sentinels. Both are out of range for any real LLM call, so the practical risk is minimal — worth a code comment or API spec note but not a blocker.

[coderabbitai]

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-api-server/plugins/agents/model.go`:
- Around line 41-52: The Agent.BeforeCreate defaulting sets LlmTemperature to
0.7 when it equals unsetTemperature (-1.0), but tests expect a default around
0.0; update the defaulting logic in BeforeCreate so that when LlmTemperature ==
unsetTemperature it is set to 0.0 (and adjust the unsetTemperature comment if
needed) to match TestAgentPostLlmDefaults and the API contract.

🪄 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: a8a068d0-b1c3-4e25-9fa3-3eefdf319126

📥 Commits

Reviewing files that changed from the base of the PR and between 0f89be7 and 98aabf4.

📒 Files selected for processing (2)

• components/ambient-api-server/plugins/agents/model.go
• components/ambient-api-server/plugins/agents/presenter.go

🚧 Files skipped from review as they are similar to previous changes (1)

• components/ambient-api-server/plugins/agents/presenter.go

[coderabbitai]

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-api-server/plugins/agents/model.go`:
- Around line 41-47: The ConvertAgent/BeforeCreate flow relies on sentinel
constants unsetTemperature and unsetMaxTokens and thus wrongly treats
client-sent -1 values as "unset" and also allows out-of-range values; add
validation in the HTTP handler/service (the Create and Patch paths, immediately
after binding the request object where agent.LlmTemperature and
agent.LlmMaxTokens are available) to reject invalid or out-of-range inputs
(e.g., require 0.0 <= temperature <= 2.0 and max_tokens > 0) and return a
BadRequest error, so ConvertAgent and BeforeCreate only see already-validated,
non-sentinel values; alternatively, if you keep sentinels, change sentinel
values to extreme impossible numbers and document them, but prefer the
handler-level validation on agent.LlmTemperature and agent.LlmMaxTokens.

🪄 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: 9e716ab7-bf02-4a25-a5e2-86a7045633aa

📥 Commits

Reviewing files that changed from the base of the PR and between 98aabf4 and 9c40c62.

📒 Files selected for processing (3)

• components/ambient-api-server/plugins/agents/integration_test.go
• components/ambient-api-server/plugins/agents/model.go
• components/ambient-api-server/plugins/agents/presenter.go

🚧 Files skipped from review as they are similar to previous changes (2)

• components/ambient-api-server/plugins/agents/presenter.go
• components/ambient-api-server/plugins/agents/integration_test.go

[markturansky]

Round 5 — LGTM ✅

Reviewed commit a0e65ec — "fix(runner): extract last assistant message, not first, from snapshot". Three files changed (+34/-193).

grpc_transport.py — _write_message() fix ✅

The root cause was next() selecting the first assistant message from the MESSAGES_SNAPSHOT accumulator. In multi-turn runs, the first assistant message is often a tool-call-only turn with content=None, so the write captured nothing. The fix iterates all accumulated messages and keeps the last one with truthy content — exactly the right semantics.

assistant_text = ""
for m in self._accumulated_messages:
    if m.get("role") == "assistant" and m.get("content"):
        assistant_text = m["content"]

Handles edge cases correctly: no assistant messages → empty string, all tool-call-only turns → empty string (no phantom write), last content-bearing message wins regardless of position.

chat-messages.tsx — filterEmptyMessages replaces enrichMessages ✅

The old enrichMessages workaround tried to back-fill empty assistant messages by fishing last_assistant_message out of adjacent hook:Stop system events — fragile coupling between two unrelated event types. The new approach is correct:

export function filterEmptyMessages(messages: DomainSessionMessage[]): DomainSessionMessage[]  {
  return messages.filter(
    (msg) => !(msg.eventType === 'assistant' && !msg.payload.trim()),
  )
}

Now that the runner fix ensures the DB record has real content, the UI simply drops any empty/whitespace-only assistant messages. Clean separation of concerns; 193 lines of workaround removed.

buildChatItems pipeline is clean: filterEmptyMessages → filter to CHAT_EVENT_TYPES → groupChatItems.

chat-messages.test.ts — coverage ✅

Tests for filterEmptyMessages cover the four meaningful cases: drop empty string, drop whitespace-only, pass through non-empty assistant, pass through non-assistant events. The buildChatItems integration test drives the full pipeline with a real multi-turn conversation shape and verifies both that empty assistant messages are filtered and that system/lifecycle events are excluded.

---

Both the runner fix and the UI simplification are correct, well-tested, and the 193-line net deletion is a welcome reduction in complexity. Nothing left to block.

[markturansky]

Round 6 — REQUEST_CHANGES

Three blockers in the amended runner commit and the new UI code.

---

BLOCKER 1 — grpc_transport.py: write-ALL regression (runner behavioral change)

The amended commit 8c70da0 changed Round 5's approved fix in a way that contradicts the commit message:

Round 5 (approved):

assistant_text = ""
for m in self._accumulated_messages:
    if m.get("role") == "assistant" and m.get("content"):
        assistant_text = m["content"]
# → single push with the LAST assistant message

Round 6 (amended):

assistant_texts = [
    m["content"]
    for m in self._accumulated_messages
    if m.get("role") == "assistant" and m.get("content")
]
# → loop: one push PER assistant message

The commit message still says "extract last assistant message, not first" but the implementation now pushes every assistant message in the snapshot. In a 3-turn conversation this writes 3 records on RUN_FINISHED instead of 1.

Why this is a problem: if earlier turns were already written to the DB during the run (e.g. from prior _write_message calls or other mechanisms), writing them all again on RUN_FINISHED creates duplicates. The UI's filterEmptyMessages does not deduplicate by content. The test suite only covers the single-turn case and won't catch this.

Additionally, the early return on the empty case was removed — the code now falls through to a logger.info("PushSessionMessages: count=0") followed by a no-op executor dispatch. Functionally harmless but misleading (log says "PushSessionMessages" even when nothing was pushed).

Fix options: (a) revert to the Round 5 last-only approach and fix the commit message, or (b) if the intent is genuinely to write the full conversation, update the commit message AND either de-duplicate in the push layer or use a replace-all call instead of per-message pushes.

---

BLOCKER 2 — app-sidebar.tsx: useSessions('') violates query-enabled-guard

// app-sidebar.tsx
const { data: sessionsData } = useSessions(projectId ?? '')

When projectId is null (user has no project selected), this passes '' to useSessions, firing a real API call with an empty project ID. This is the same pattern that blocked PR #1636 (query-enabled-guard in amber's pattern catalogue).

Fix:

const { data: sessionsData } = useSessions(projectId ?? '', { enabled: !!projectId })

---

BLOCKER 3 — TestToolbar in chat-sidebar.tsx: empty projectId breaks re-run

// TestToolbar
createSession.mutate(
  {
    name: newName,
    projectId: '', // will be derived from the agent ← incorrect
    agentId: activeSession.agentId,
    ...
  },
  ...
)

projectId: '' will fail at the API layer — the server requires a non-empty project ID. The comment "will be derived from the agent" is incorrect; no derivation happens here. The parent ChatSidebar already extracts projectId from useParams — either pass it as a prop to TestToolbar or have TestToolbar call useParams itself. Compare NewSessionButton in the same file, which correctly receives and uses projectId.

There is also no onError handler on this createSession.mutate call — a failed re-run silently swallows the error. Add a toast.error(...) in onError.

---

NOTABLE — TabStrip: nested interactive element (a11y)

Each tab button contains a close <span role="button"> nested inside a <button>:

<button ... onClick={() => onSwitch(s.sessionId)}>
  ...
  <span role="button" tabIndex={0} onClick={e => { e.stopPropagation(); onClose(s.sessionId) }}>
    <X ... />
  </span>
</button>

Interactive elements may not be nested inside <button> elements per ARIA authoring practices. Consider changing the tab container from <button> to a focusable <div role="tab"> and restoring keyboard semantics explicitly, or keeping the close X as a sibling <button> positioned absolutely.

---

NOTABLE — create-agent-sheet.tsx: raw error message exposed

catch (err) {
  setError(err instanceof Error ? err.message : 'Failed to create agent.')
}

This exposes raw API error messages to the user — inconsistent with the "Generic error message on dashboard" fix in the 14 CodeRabbit fixes commit. Use a generic message and log the detail:

catch (err) {
  console.error('create agent failed', err)
  setError('Failed to create agent. Please try again.')
}

---

MINOR — no tests for dashboard-helpers.ts pure functions

getAttentionItems, getActiveWorkItems, and getRecentActivity are pure functions with non-trivial logic (phase filtering, grouping by work-item key, sort-by-completionTime). They're easy to unit test and the query-enabled-guard and deferred-data-fetch findings from prior PRs were caught by missing coverage exactly like this. A small test file would prevent regressions as the dashboard evolves.

---

The rest of the new UI surface looks solid: chat-sidebar-context.tsx state management is correct (deps array ensures fresh sessions in closeSession), dashboard-helpers.ts correctly guards the needs-input !== 'false' annotation edge case, use-table-keyboard-nav.ts and use-escape-back.ts both properly guard against input focus and open dialogs, and bulk-action-bar.tsx correctly surfaces partial failures via toast.error. The three blockers need to be resolved before this can merge.

[markturansky]

Round 7 — REQUEST_CHANGES (2 blockers remaining)

Runner fix — LGTM ✅

The GRPCMessageWriter rewrite is correct. Switching from a snapshot-reconstruction approach to streaming TEXT_MESSAGE_START/CONTENT/END events resolves all three issues flagged in Round 6:

• Each assistant turn is pushed exactly once, at the moment TEXT_MESSAGE_END fires
• No end-of-run reconstruction from MESSAGES_SNAPSHOT needed — eliminates the write-all vs write-last ambiguity entirely
• The if text: guard in _push_assistant means empty turns are never pushed — filterEmptyMessages in the UI is now defense-in-depth rather than a primary workaround
• Docstring updated to match the new semantics

The _text_buffer accumulation pattern (START → CONTENT deltas → END → push) is the same pattern used by TOOL_CALL_START/ARGS/END and confirmed correct in PR #1636.

---

BLOCKER 1 — app-sidebar.tsx: useSessions('') not fixed

const { data: sessionsData } = useSessions(projectId ?? '')

Unchanged from Round 6. Passes '' to useSessions when projectId is null, firing a real API call with an empty project ID. Fix:

const { data: sessionsData } = useSessions(projectId ?? '', { enabled: !!projectId })

---

BLOCKER 2 — TestToolbar in chat-sidebar.tsx: projectId: '' not fixed

createSession.mutate(
  {
    name: newName,
    projectId: '', // will be derived from the agent ← still broken
    agentId: activeSession.agentId,
    ...
  },
)

Unchanged from Round 6. The parent ChatSidebar now correctly extracts projectId via useParams — pass it as a prop to TestToolbar or have TestToolbar call useParams itself.

---

Two lines to fix. Everything else is clean.

[markturansky]

Round 8 — REQUEST_CHANGES

Fixed since Round 7 ✅

• TestToolbar projectId — now correctly received as a prop from ChatSidebar via useParams. Re-run flow is fixed.
• create-agent-sheet.tsx — generic error message used; raw err.message no longer exposed to the user.
• useSessions guard — the hook has enabled: !!projectId internally. useSessions(projectId ?? '') was always safe; the call site concern was resolved at the hook layer.

---

Blocker: _synthesize_run_error calls writer._write_message() — method no longer exists

grpc_transport.py, _synthesize_run_error:

task = asyncio.ensure_future(writer._write_message(status="error"))

GRPCMessageWriter was completely rewritten in Round 7 to stream on TEXT_MESSAGE_* events. It now only has consume and _push_assistant — _write_message was removed but _synthesize_run_error was not updated. On any bridge.run() exception:

1. _synthesize_run_error runs — the SSE RunErrorEvent is pushed to the queue ✓
2. writer._write_message(status="error") raises AttributeError immediately
3. The AttributeError propagates out of the except block in _handle_user_message, through the finally, into _listen_loop's outer handler
4. _listen_loop treats it as a connection error and reconnects with exponential backoff — wrong behavior for a run error
5. No error record is written to DB

Fix — remove the dead call:

# _synthesize_run_error: delete this line
task = asyncio.ensure_future(writer._write_message(status="error"))

The error is already logged via logger.error in _handle_user_message and the SSE event is delivered. If DB error records are still needed, add a write_error() method to GRPCMessageWriter.

The module-level docstring is also stale — it still reads "Accumulates MESSAGES_SNAPSHOT content. Pushes one PushSessionMessage on RUN_FINISHED / RUN_ERROR." Update it to reflect the streaming design.

---

Draft persistence (e6b26766) ✅

readDraft/saveDraft/clearDraft is clean: 48-hour expiry checked on read, per-session ambient-draft:${sessionId} key, try/catch for quota exceeded and SSR (localStorage not defined). Lazy useState initializer is correct. useEffect on [sessionId] restores the right draft on session switch. clearDraft on onSuccess is the right place.

---

Notable (carry-forward, not blocking)

TabStrip in chat-sidebar.tsx: <span role="button" tabIndex={0}> nested inside <button role="tab"> — ARIA authoring practices violation, screen readers will misinterpret the close action. Worth a follow-up before wider rollout.

---

One fix: remove the dead _write_message call from _synthesize_run_error and update the module docstring to match the streaming implementation.

[markturansky]

Round 9 — LGTM ✅

Fixed since Round 8

_synthesize_run_error crash — push_error added to GRPCMessageWriter. Implementation is correct:

• async def push_error(error_message) flushes any partial _text_buffer first (handles exception mid-stream), then pushes an event_type="error" record
• Exception handling on the push itself ✅
• _synthesize_run_error now calls asyncio.ensure_future(writer.push_error(error_message)) with the _log_write_error callback — correct pattern ✅
• Fire-and-forget error record doesn't propagate AttributeError to _listen_loop anymore ✅

---

Carry-forward notable (non-blocking)

Module-level docstring at the top of grpc_transport.py still reads:

GRPCMessageWriter — per-turn event consumer.
  Accumulates MESSAGES_SNAPSHOT content.
  Pushes one PushSessionMessage(event_type="assistant") on RUN_FINISHED / RUN_ERROR.

Doesn't match the streaming implementation. Update in a follow-up.

TabStrip <span role="button"> nested inside <button role="tab"> — ARIA violation, worth a follow-up before wider rollout.

---

Everything else reviewed across 9 rounds is clean. Backend (GORM migration, sentinels, integration tests), runner (TEXT_MESSAGE streaming, error path), and UI (multi-session sidebar, draft persistence, keyboard nav, dashboard, agent CRUD) — all good. Approving.

[markturansky]

Amber Review — PR #1647 (ambient-api-server changes, post-merge)

Focused on components/ambient-api-server/plugins/agents/: handler, model, migration, presenter, integration tests.

---

Issues (on main now)

C1 — 404 opacity invariant violated in Get, Patch, Delete (Critical)

handler.go returns 403 Forbidden when an agent exists but belongs to a different project:

if found.ProjectId != projectID {
    return nil, errors.Forbidden("agent does not belong to this project")
}

This leaks agent ID existence across project boundaries. A caller can probe any agent ID through any project endpoint and distinguish "agent doesn't exist" (404) from "agent exists in another project" (403). The correct response is 404 in both cases — same class of issue as B1 from PR #1640 (which also landed on main unfixed).

The test TestAgentCrossProjectIsolation hardcodes and validates the broken behavior (Equal(http.StatusForbidden)), which will need updating alongside the fix.

Fix: return 404 in the cross-project branch:

if found.ProjectId != projectID {
    return nil, errors.NotFound("Agent", "id", id)
}

M1 — TOCTOU race in PATCH read-modify-write (Major)

Patch reads the agent with h.agent.Get, modifies in-process, then calls h.agent.Replace. The advisory lock in Replace covers only the write, not the full read-modify-write cycle:

Thread A: GET agent (temperature=0.7)
Thread B: GET agent (temperature=0.7)          ← both read before either lock
Thread A: patch temperature to 0.5 → Replace (lock, write, unlock)
Thread B: patch displayName → Replace (lock, write 0.7, unlock) ← A's change lost

Low likelihood in production (concurrent patches to the same agent are rare) but worth tracking. Fix requires acquiring the advisory lock before the Get, or using a database-level optimistic lock (version/etag field).

---

What's good

Sentinel pattern is correct. Using unsetTemperature = -1.0 and unsetMaxTokens = -1 in model.go cleanly separates "caller explicitly sent 0.0" from "caller omitted the field." The TestAgentPatchTemperatureZeroPreserved test validates this edge case explicitly — good coverage.

Migration is safe. ADD COLUMN IF NOT EXISTS is idempotent; existing rows get sensible defaults (owner_user_id NOT NULL DEFAULT '', llm_model DEFAULT 'claude-sonnet-4-6', etc.). Rollback drops only the added columns, not the whole table.

Integration test coverage is solid. 9 tests covering: GET (with LLM default assertions), POST, POST with defaults, PATCH, temperature=0.0 edge case, paging, search, DELETE, and cross-project isolation. This is the right level of coverage for a CRUD handler. Removing the t.Skip() stubs and shipping real tests is the right call.

PATCH field coverage is complete. All patchable fields (DisplayName, Description, RepoUrl, LlmModel, LlmTemperature, LlmMaxTokens, Labels, Annotations) are wired through the handler. Previously only Name and Prompt were persisted — this fixes the regression cited in the PR description.

---

Minor

• LlmTemperature is not range-checked in Create or PATCH — callers can store values outside [0.0, 2.0] without error
• agent_id path variable is not validated against validIDPattern in Get/Patch/Delete, inconsistent with projectID validation in List
• fmt.Errorf("agents.Create: %s", err.Error()) in factory_test.go should use %w to preserve the error chain (test-only, non-blocking)

---

Summary

Two issues landed on main: the 404 opacity violation (C1) repeats the same class of gap from PR #1640 — worth consolidating into a single follow-up issue since it now affects agents, sessions, and likely other resources. The PATCH TOCTOU (M1) is low-urgency but real. Everything else is clean work.

— Amber