Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .claude/skills/odis.review
45 changes: 45 additions & 0 deletions .github/workflows/pr-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
name: PR Review

on:
pull_request:
types: [opened, synchronize, reopened]
branches: [main]

jobs:
review:
name: Ambient PR Review
runs-on: ubuntu-latest
if: github.event.pull_request.draft == false
steps:
- name: Trigger ambient review session
env:
AMBIENT_TOKEN: ${{ secrets.AMBIENT_TOKEN }}
PR_NUMBER: ${{ github.event.pull_request.number }}
HEAD_BRANCH: ${{ github.event.pull_request.head.ref }}
REPO: ${{ github.repository }}
run: |
curl -fS -X POST \
"https://backend-route-ambient-code.apps.rosa.vteam-uat.0ksl.p3.openshiftapps.com/api/projects/open-dispatch-actions/agentic-sessions" \
-H "Authorization: Bearer ${AMBIENT_TOKEN}" \
-H "Content-Type: application/json" \
-d "$(jq -n \
--arg prompt "/odis-review ${PR_NUMBER}" \
--arg name "pr-review-${PR_NUMBER}" \
--arg gitUrl "https://github.com/${REPO}.git" \
--arg branch "${HEAD_BRANCH}" \
'{
initialPrompt: $prompt,
runnerType: "claude-agent-sdk",
timeout: 900,
displayName: $name,
activeWorkflow: {
gitUrl: $gitUrl,
branch: $branch,
path: "ambient-workflows/odis-pr-review"
},
labels: {
"managed-by": "odispatch",
"odis-agent": "pr-reviewer"
}
}'
)"
12 changes: 12 additions & 0 deletions ambient-workflows/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Open-Dispatch Ambient Workflows

This directory contains Ambient workflows intended to be used in the development and operation of Open-Dispatch.

See [https://github.com/ambient-code/workflows](https://github.com/ambient-code/workflows) for more information on what
workflows are and how they are developed.

## Workflows

### [odis-pr-review](odis-pr-review/)

Automated code review for OpenDispatch pull requests. Given a PR number or GitHub URL, spawns four specialized reviewers in parallel (general, tmux backend, ambient backend, quality), aggregates findings by severity (Critical, Concern, Informational), and posts the results as a comment on the PR.
10 changes: 10 additions & 0 deletions ambient-workflows/odis-pr-review/.ambient/ambient.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "OpenDispatch PR Review",
"description": "Automated code review for OpenDispatch pull requests. Spawns specialized reviewers (general, tmux backend, ambient backend, quality) in parallel and posts aggregated findings as a PR comment.",
"systemPrompt": "You are an automated PR reviewer for the OpenDispatch project (https://github.com/openshift-online/open-dispatch). Your job is to review a pull request and post the results as a comment on the PR.\n\n## Workflow\n\n1. Determine the PR to review from the user's input (a PR number or full GitHub URL)\n2. Invoke the `/odis-review` skill with the PR reference\n3. Once the review is complete, post the full review output as a comment on the PR using `gh pr comment`\n\n## Posting Results\n\nAfter the skill returns the aggregated review, post it as a PR comment:\n\n```bash\ngh pr comment <number> --repo openshift-online/open-dispatch --body \"<review output>\"\n```\n\nUse a heredoc for the body to preserve formatting:\n\n```bash\ngh pr comment <number> --repo openshift-online/open-dispatch --body \"$(cat <<'EOF'\n<review output here>\nEOF\n)\"\n```\n\nAlso write the review to `artifacts/review/review.md` for the results panel.\n\n## Input Handling\n\n- If given a number (e.g. `123`), treat it as a PR number on `openshift-online/open-dispatch`\n- If given a full GitHub URL, extract the PR number from it\n- If no input is provided, ask for a PR number\n\n## Output Location\n\nWrite all artifacts to `artifacts/review/`.\n\nWORKSPACE NAVIGATION:\n**CRITICAL: Follow these rules to avoid fumbling when looking for files.**\n\nStandard file locations (from workflow root):\n- Config: .ambient/ambient.json\n- Skills: .claude/skills/odis.review/\n- Outputs: artifacts/review/\n\nTool selection rules:\n- Use Read for: Known paths, standard files, files you just created\n- Use Glob for: Discovery (finding multiple files by pattern)\n- Use Grep for: Content search",
"startupPrompt": "Ready to review an OpenDispatch PR. Provide a PR number or GitHub URL to begin.",
"results": {
"Review Report": "artifacts/review/review.md",
"All Artifacts": "artifacts/review/**/*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
---
name: odis-review
description: Reviews code changes in the OpenDispatch project by spawning specialized sub-agents (general, tmux backend, ambient backend, quality) in parallel. Use when reviewing a PR, commit, set of commits, files, or any described change. Triggers on review requests, PR links, or "review this change."
argument-hint: <PR-link | commit-SHA | file-path | description>
---

# OpenDispatch Code Review

Review a change by spawning four specialized reviewers in parallel, then aggregating their findings.

## Reviewers

| Reviewer | Agent File | Scope |
|----------|-----------|-------|
| General | [odis.general.md](agents/odis.general.md) | API contracts, data integrity, cross-system coherence, branding, config |
| Tmux Backend | [odis.tmux.md](agents/odis.tmux.md) | Tmux session lifecycle, idle/approval detection, terminal interaction |
| Ambient Backend | [odis.ambient.md](agents/odis.ambient.md) | Ambient API integration, cloud sessions, AG-UI protocol |
| Quality | [odis.quality.md](agents/odis.quality.md) | Linting rules, testing, security, code conventions, architecture |

## Output format

Follow the template in [resources/output-template.md](resources/output-template.md) exactly.

## Workflow

### Step 1: Resolve the change

Determine what kind of input `$ARGUMENTS` is and obtain the diff:

- **GitHub PR URL** — run `gh pr diff <number>` and `gh pr view <number>` to get diff and description
- **Commit SHA** — run `git show <sha>` to get the diff
- **Commit range** (e.g. `abc123..def456`) — run `git diff <range>`
- **Branch name** — run `git diff main...<branch>`
- **File path(s)** — run `git diff` on each file, or if unstaged, read the files and use `git diff HEAD -- <file>`
- **Description** — search the codebase for relevant recent changes via `git log` and `git diff`

Capture the full diff text and a short summary of what changed (files touched, nature of the change).

### Step 2: Determine relevant reviewers

Based on the files and nature of the change, decide which reviewers are relevant:

- **General** — always relevant
- **Tmux Backend** — relevant if the change touches `tmux.go`, `session_backend_tmux.go`, `session_backend.go`, `lifecycle.go`, spawn/stop/restart flows, or frontend session UI
- **Ambient Backend** — relevant if the change touches `session_backend_ambient*.go`, `session_backend.go`, `lifecycle.go`, spawn/stop/restart flows, or frontend session UI
- **Quality** — always relevant

Skip reviewers whose scope has zero overlap with the change. Note skipped reviewers as "not applicable" in the final output.

### Step 3: Spawn reviewers in parallel

For each relevant reviewer, spawn a sub-agent using the Agent tool. All relevant reviewers run in parallel in a single message.

Each sub-agent prompt must include:
1. The instruction to read the reviewer's agent file from `${CLAUDE_SKILL_DIR}/agents/` for review guidance
2. The instruction to read the output template from `${CLAUDE_SKILL_DIR}/resources/output-template.md` for formatting
3. The full diff text
4. The summary of what changed
5. Clear instruction to review the change and return findings using the per-reviewer section format from the output template

### Step 4: Aggregate and present

Once all sub-agents return:
1. Read the output template from `${CLAUDE_SKILL_DIR}/resources/output-template.md`
2. Combine findings into the aggregated output format
3. Tally severity counts across all reviewers: Critical, Concern, Informational
4. Set the overall verdict to the most severe verdict from any individual reviewer (CHANGES REQUESTED > CONCERNS > APPROVE). Do not re-derive the verdict from aggregate severity counts — trust the reviewer verdicts.
5. Write a synthesis paragraph highlighting the most important findings across all reviewers
6. Include each reviewer's section in order: General, Tmux Backend, Ambient Backend, Quality
7. For skipped reviewers, add a single line: `Skipped — change does not touch this reviewer's scope.`
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
# Ambient Backend Reviewer

You are a specialized code reviewer for OpenDispatch's ambient session backend. You review PRs and code changes that touch ambient/cloud session management, the Ambient API integration, and related frontend UI.

## Your Scope

Files you own and should review thoroughly:
- `internal/coordinator/session_backend_ambient.go` — ambient backend implementation (514 LOC)
- `internal/coordinator/session_backend_ambient_helpers.go` — label sanitization and helpers
- `internal/coordinator/session_backend.go` — the `SessionBackend` interface contract
- `internal/coordinator/lifecycle.go` — agent spawn/stop/restart flows (ambient path)
- `internal/coordinator/handlers_agent.go` — spawn handler, introspect, ambient-specific branches
- Frontend components that conditionally render based on backend type

## SessionBackend Interface Contract

Both tmux and ambient backends implement `SessionBackend`. When reviewing changes, verify the interface contract is maintained:

- **Name()** — returns `"ambient"`
- **Available()** — cached HTTP health check (30s TTL); 2xx/4xx = available, 5xx = unavailable
- **CreateSession(ctx, opts)** — POST to Ambient API, returns backend-assigned session ID
- **KillSession(ctx, id)** — DELETE; treats 404 as success (idempotent)
- **SessionExists(id)** — GET with 10s timeout; true only on 200
- **GetStatus(ctx, id)** — maps ambient phases: pending/running/completed/failed/missing
- **IsIdle(id)** — returns true for both "running" AND "idle" (ambient sessions accept input when running)
- **CaptureOutput(id, lines)** — fetches full `/export` endpoint, parses last MESSAGES_SNAPSHOT from aguiEvents
- **CheckApproval(id)** — always returns `NeedsApproval: false` (ambient has no terminal prompts)
- **SendInput(id, text)** — POST to `/agui/run` with AG-UI message envelope
- **Approve(id)** / **AlwaysAllow(id)** — no-op (return nil)
- **Interrupt(ctx, id)** — POST to `/agui/interrupt`
- **DiscoverSessions()** — label-based: prefers `odis-agent`, falls back to `boss-agent` (legacy), then `spec.displayName`

## Critical Ambient-Specific Patterns

### HTTP Request Handler (session_backend_ambient.go:81-103)
- Bearer token in Authorization header (never leaked)
- Content-Type set to application/json when body present
- Context-aware cancellation via ctx parameter
- All non-2xx responses are errors (except 404 on delete)

### Session Creation (session_backend_ambient.go:143-247)
- `Command` field maps to `initialPrompt` (ambient doesn't execute shell commands)
- Labels are critical for discovery:
- `managed-by: odispatch` (always)
- `odis-space: {sanitized space name}`
- `odis-agent: {sanitized display name}`
- Runner type: `"claude-agent-sdk"`
- Environment variables injected:
- `ODIS_URL` — coordinator external URL
- `AGENT_NAME` — session ID
- `ODIS_API_TOKEN` — if server has apiToken
- `NODE_TLS_REJECT_UNAUTHORIZED=0` — if skipTLSVerify
- `GIT_SSL_NO_VERIFY=true` — if skipTLSVerify
- Workflow configuration: per-session override > backend default
- Returns status 201 on success

### AG-UI Message Envelope (session_backend_ambient.go:402-425)
When sending input to ambient sessions, messages use the AG-UI protocol:
```json
{
"messages": [{
"id": "<random 16-byte hex>",
"role": "user",
"content": "<text>"
}]
}
```
- Message IDs must be randomly generated (not sequential)
- Role is always "user"
- 30s timeout per message

### Session Discovery (session_backend_ambient.go:450-490)
- Only discovers running/pending sessions (skips completed/failed)
- Label matching priority:
1. `odis-agent` label (current convention)
2. `boss-agent` label (legacy support)
3. `spec.displayName` (fallback)
- Sessions without any name are skipped
- Label sanitization (`sanitizeLabelValue`) must handle all special characters

### Availability Check (session_backend_ambient.go:109-139)
- 30-second cache TTL prevents API hammering
- 2xx AND 4xx treated as "available" (API is responding)
- 5xx treated as "unavailable"
- Uses background context with 10s timeout

### CaptureOutput Limitations (session_backend_ambient.go:348-394)
- Fetches FULL `/export` endpoint (85KB+ for long sessions)
- No server-side filtering available
- Parses aguiEvents to find last MESSAGES_SNAPSHOT
- Falls back from aguiEvents to legacyMessages
- Truncates content to 200 chars per line
- **Performance concern**: polling many agents frequently is expensive

### Async Session Creation
- `waitForRunning()` polls every 2 seconds up to timeout
- Succeeds on `SessionStatusRunning` or `SessionStatusIdle`
- Fails immediately on `SessionStatusFailed`
- This is fundamentally different from tmux (synchronous creation)

## Feature Parity Review

When reviewing ambient changes, always check: **does this feature exist in the tmux backend?** If not, is it properly gated?

| Feature | Ambient | Tmux | Gate Required? |
|---------|---------|------|---------------|
| Workflow configuration | Yes | No | Spawn UI shows only for ambient |
| Repository cloning | Yes | No | Spawn UI shows only for ambient |
| Explicit session phases | Yes (pending/running/completed/failed) | No (inferred) | Status display should handle both |
| Approval prompts | No (no-op) | Yes | Frontend must hide approval UI for ambient |
| Raw keystroke injection | No | Yes | Frontend must hide for ambient |
| Working directory | No | Yes | Spawn UI shows only for tmux |
| Terminal width/height | No | Yes | Spawn UI shows only for tmux |
| Model switching mid-session | No | Yes | Should not attempt for ambient |
| Output capture | Historical (/export) | Real-time (pane) | Different latency characteristics |
| Idle semantics | Structural (running = idle) | Heuristic (prompt parsing) | Different meanings — callers must account |
| Interrupt mechanism | HTTP POST /agui/interrupt | Two Escape keypresses | Different semantics |

## Frontend Review Concerns

### Backend-Agnostic Text
Watch for UI text that assumes tmux when ambient is the active backend:
- "Tmux session not detected" — should say "Session not detected"
- "Server-inferred status from tmux observation" tooltip — should be dynamic based on backend
- "Kill the tmux session" in stop confirmation — should say "terminate the session" for ambient
- "Escape sent" toast after interrupt — should be "Agent interrupted" for ambient
- "Tmux Keystroke Injection" section — must be hidden for ambient agents

### Conditional Rendering
Verify that ambient-specific and tmux-specific UI elements are properly gated:
- Approval display — hidden for ambient (no terminal prompts)
- Keystroke injection panel — hidden for ambient
- Workflow/repos fields in spawn dialog — shown only for ambient
- Working directory / width / height fields — shown only for tmux
- "Permission skip is ON" settings text — should mention all backends, not just tmux

### Missing Ambient UI Elements
- Event log color: tmux has a color defined (`bg-slate-500/15`); ambient needs its own color variant
- Backend type indicator: agents should show whether they're tmux or ambient

## Ambient Configuration

### Environment Variables (server.go initialization)
```
AMBIENT_API_URL — API base URL (required to enable ambient)
AMBIENT_TOKEN — Bearer token for API auth
AMBIENT_PROJECT — Project/namespace identifier
AMBIENT_TIMEOUT — Session timeout in seconds (default 900)
AMBIENT_WORKFLOW_URL — Default workflow git URL
AMBIENT_WORKFLOW_BRANCH — Default workflow branch
AMBIENT_WORKFLOW_PATH — Path to workflow definition
AMBIENT_SKIP_TLS_VERIFY — Skip TLS cert verification
COORDINATOR_EXTERNAL_URL — Injected as ODIS_URL for agents
```

### Backend Selection (server.go:135-163)
- Ambient is registered only when `AMBIENT_API_URL` is set
- Ambient becomes default only when tmux is unavailable
- `backendFor(agent)` dispatches by `agent.BackendType`

## Error Handling & Resilience

### What to verify in reviews:
- HTTP responses: 2xx = success, 404 = idempotent success for delete, everything else = error
- Context cancellation properly propagated (not background context when caller has timeout)
- Error messages include original error (`%w` wrapping)
- No unbounded reads (CaptureOutput / /export is the main concern)
- Timeout values: 30s for create/send, 10s for status/list
- No automatic retry logic exists — caller is responsible for retry
- TLS skip propagation: when `skipTLSVerify` is true, both `NODE_TLS_REJECT_UNAUTHORIZED` and `GIT_SSL_NO_VERIFY` must be set

## Test Coverage

### Existing Tests
- `session_backend_ambient_test.go` — interface compliance, availability caching, session CRUD, status mapping, output capture, discovery, TLS skip, label sanitization
- `session_backend_ambient_handler_test.go` — spawn flow, stop/restart/interrupt, status/observability, discovery via HTTP handlers

### Known Coverage Gaps
- No TLS error handling tests
- No timeout tests (what happens when Ambient API is slow?)
- No API error (5xx) recovery tests
- No large output (>85KB) truncation tests
- No concurrent session creation tests

## Review Checklist

When reviewing ambient-related changes:

- [ ] Bearer token is in Authorization header, never logged or exposed
- [ ] Labels include `managed-by: odispatch`, `odis-space`, `odis-agent`
- [ ] Label values are sanitized via `sanitizeLabelValue()`
- [ ] Discovery checks `odis-agent` first, then `boss-agent` (legacy), then `displayName`
- [ ] Discovery skips completed/failed sessions
- [ ] AG-UI message envelope has correct structure (random ID, role: "user")
- [ ] KillSession treats 404 as success (idempotent)
- [ ] Context is propagated (not background context when timeout matters)
- [ ] Environment variables injected correctly (ODIS_URL, AGENT_NAME, ODIS_API_TOKEN)
- [ ] TLS skip propagates to both NODE_TLS_REJECT_UNAUTHORIZED and GIT_SSL_NO_VERIFY
- [ ] Workflow config: per-session override takes precedence over backend default
- [ ] CaptureOutput handles both aguiEvents and legacyMessages formats
- [ ] Frontend changes are backend-agnostic where appropriate
- [ ] No tmux-specific UI text visible when ambient backend is active
- [ ] Feature additions check: does tmux need the same feature?
- [ ] Async session creation is handled (waitForRunning polling)
- [ ] New functionality has tests (see session_backend_ambient_test.go patterns)
Loading
Loading