Codecov Report

❌ Patch coverage is 78.55478% with 92 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@bab6023). Learn more about missing BASE report.

@@           Coverage Diff           @@
##             main     #108   +/-   ##
=======================================
  Coverage        ?   91.68%           
=======================================
  Files           ?       43           
  Lines           ?     3441           
  Branches        ?        0           
=======================================
  Hits            ?     3155           
  Misses          ?      286           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Independent Review & Test Results

Reviewed and tested locally on Amazon Linux 2 (Python 3.12.3, tmux next-3.6).

Test Results

Review Notes

What's good:

• Claude Code env var filtering (CLAUDE*/CODEX_*) prevents nested session errors — smart fix
• Startup prompt handling now covers both trust dialog and bypass permissions warning
• Settings service is clean with proper defaults-merge-with-saved logic
• Agent profile discovery across multiple directories with deduplication works well
• WebSocket terminal streaming with PTY, resize support, and proper cleanup
• Session delete now cascades properly (kills windows, non-blocking provider cleanup)
• resolve_provider() for cross-provider orchestration is clean

Verified Concerns

1. PROCESSING_PATTERN change — ✅ Not an issue

Tested the new pattern r"[✶✢✽✻✳].*…" against spinner lines and response text. The spinner characters are specific Unicode symbols that don't appear in normal response text, so no false positives. The change correctly supports the minimal ✻ Orbiting… format that the old pattern missed.

2. WebSocket endpoint has no authentication — ⚠️ Confirmed risk

/terminals/{terminal_id}/ws gives full PTY access with zero auth. CORS does not protect WebSocket connections (browsers don't enforce same-origin on WS upgrades). The README shows cao-server --host 0.0.0.0 as an example — with that, anyone on the network gets full terminal access. Suggest at minimum a warning in docs, or better, a token/secret check on the WebSocket handshake.

3. Flow name path traversal — ⚠️ Confirmed bug

POST /flows with name: "../../etc/cron" writes to ~/.aws/etc/cron.flow.md, escaping the flows directory:

name="../../etc/cron"  -> ~/.aws/etc/cron.flow.md  ⚠️ ESCAPES
name="../../../tmp/evil" -> ~/tmp/evil.flow.md      ⚠️ ESCAPES

Fix: reject names containing / or .., or sanitize with Path(name).name.

Minor Notes

• Codecov shows 68.66% patch coverage (131 lines missing, mostly in api/main.py). WebSocket endpoint and several new API endpoints lack test coverage.
• constants.KIRO_AGENTS_DIR mutation in main() is fragile — code that imported the value before main() runs gets the old value.
• Settings file has no locking (low risk for single-user tool).
• list_agent_profiles() does lazy import on every call to avoid circular imports — consider caching.

Verdict

Solid feature PR. The path traversal bug (#3) should be fixed before merge. The WebSocket auth (#2) is worth discussing — acceptable for localhost-only use but risky if exposed.

LGTM with the path traversal fix 👍

@abdullahoff Great work on this PR! The web dashboard is a really solid addition, session-grouped view, live status badges, terminal viewer, flows, and settings all look well thought out. The backend changes are clean too: settings service with proper defaults-merge-with-saved, cascading session delete, agent profile discovery across multiple directories with dedup. Nice job on the 233 new tests as well.

A few items to address before we can merge:

1. grep -oP breaks on macOS (bug)

unset_cmd = "unset $(env | grep -oP '^CLAUDE[A-Z_]*(?==)') 2>/dev/null"

grep -P (Perl regex) is not available on macOS — it ships with BSD grep. This silently fails to unset CLAUDE* env vars on Mac, which is a primary dev platform. The 2>/dev/null suppresses the error so it will fail silently.

Fix: Use POSIX-compatible grep:

unset_cmd = "unset $(env | grep -o '^CLAUDE[A-Z_]*' | tr '\\n' ' ') 2>/dev/null"

Or use sed: env | sed -n 's/^\(CLAUDE[A-Z_]*\)=.*/\1/p'

2. Vite dev server binds 0.0.0.0

vite.config.ts has host: '0.0.0.0' which exposes the dev server (and its API proxy, including the WebSocket terminal endpoint) to the network. Since the WebSocket gives full PTY access, this is risky during development.

Fix: Consider change to host: 'localhost' (or 127.0.0.1). Developers on remote machines can use SSH tunneling as you already document in the README.

3. No frontend tests or type checking in CI

The web-build CI job only runs npm run build. Consider adding at minimum npx tsc --noEmit to catch TypeScript errors, and ideally a test step if you add tests later.

@abdullahoff LGTM, except the conflicts to be resolved. Note that PR #124 fixed a critical security bug, i can see that you have app.add_middleware conflicts in src/cli_agent_orchestrator/api/main.py for some of the origins and trusted hosts. Let's make sure we address this last conflict. Happy to approve once conflicts are properly resolved.

@fanhongy can you pls also review the conflicts to make sure that the PR #124 are properly addressed too in this PR.

Feel free to review the suggestions

Critical Issue: Test Files Will Break

4 new test files in PR #108 define their own local client() fixture that returns a plain TestClient(app):

In pytest, a local fixture overrides the conftest fixture. These files will NOT use TestClientWithHost from test/api/conftest.py (added by PR #124). The plain TestClient sends Host: testserver by default, which TrustedHostMiddleware rejects as 400.

Result: All tests in these 4 files will fail after rebase with 400 Bad Request on every API call.

Fix: Remove all 4 local client() fixtures — let them inherit from test/api/conftest.py.

File-by-File Resolution

1. src/cli_agent_orchestrator/constants.py — CONFLICT

Main (post-#124):

• SERVER_HOST = "localhost" (unchanged)
• CORS_ORIGINS (2 entries)
• ALLOWED_HOSTS (new from fix(api): add DNS rebinding protection via Host header validation #124)

PR #108 wants:

• SERVER_HOST → configurable via os.environ.get("CAO_API_HOST", "127.0.0.1")
• SERVER_PORT → configurable via env var
• CORS_ORIGINS → add Vite dev server origins (:5173)
• CAO_HOME_DIR, COPILOT_AGENTS_DIR constants
• No ALLOWED_HOSTS (predates fix(api): add DNS rebinding protection via Host header validation #124)

Resolution — take both:

import os
from pathlib import Path

SERVER_HOST = os.environ.get("CAO_API_HOST", "127.0.0.1")
SERVER_PORT = int(os.environ.get("CAO_API_PORT", "9889"))
SERVER_VERSION = "0.1.0"
API_BASE_URL = f"http://{SERVER_HOST}:{SERVER_PORT}"

CORS_ORIGINS = [
    "http://localhost:3000",
    "http://127.0.0.1:3000",
    "http://localhost:5173",
    "http://127.0.0.1:5173",
]

# Keep PR #124's ALLOWED_HOSTS exactly as-is
ALLOWED_HOSTS = [
    "localhost",
    "127.0.0.1",
]

2. src/cli_agent_orchestrator/api/main.py — CONFLICT

Main (post-#124): Has TrustedHostMiddleware import, ALLOWED_HOSTS import, middleware registration.

PR #108 wants: Adds CORSMiddleware, WebSocket endpoint, flow endpoints, settings endpoints, static file serving, argparse in main().

Resolution — keep both middlewares, order matters:

from fastapi.middleware.cors import CORSMiddleware
from fastapi.middleware.trustedhost import TrustedHostMiddleware

# TrustedHostMiddleware MUST be added first (runs outermost in Starlette)
# so it rejects bad Host headers before CORS processing
app.add_middleware(
    TrustedHostMiddleware,
    allowed_hosts=ALLOWED_HOSTS,
)
app.add_middleware(
    CORSMiddleware,
    allow_origins=CORS_ORIGINS,
    allow_credentials=True,
    allow_methods=["*"],
    allow_headers=["*"],
)

Also keep PR #124's ALLOWED_HOSTS import alongside PR #108's new imports (CAO_HOME_DIR, CORS_ORIGINS, etc.).

3. test/api/conftest.py — CONFLICT (identical content)

Both PRs create this file with the same TestClientWithHost class. Already on main from #124.

Resolution: Keep main's version. Drop PR #108's version during rebase.

4. test/api/test_terminals.py — CONFLICT (same removal)

Both PRs remove the local client() fixture. Already removed on main.

Resolution: Keep main's version. The PR #108 additions (new test classes) apply cleanly on top.

5. test/api/test_inbox_messages.py — CONFLICT (same removal)

Same as test_terminals.py.

Resolution: Keep main's version.

6. README.md — LIKELY CONFLICT

Main now has "DNS Rebinding Protection" subsection under Security (from #124). PR #108 adds WebSocket security note + Web UI section.

Resolution: Keep both. The Security section should contain both the DNS rebinding note and the WebSocket note.

7. test/api/test_api_coverage.py — NO GIT CONFLICT but BROKEN

New file from PR #108. No git conflict, but the local client() fixture will break.

Resolution: Remove local client() fixture, rely on conftest.

Note: The test_websocket_rejects_non_loopback test expects a failure with Host: testserver. With TestClientWithHost sending Host: localhost, TrustedHostMiddleware will pass — but the WebSocket IP check (client_host not in ("127.0.0.1", ...)) may still reject. Verify this test still passes after the fixture change.

8. test/api/test_api_endpoints.py, test_flow_api.py, test_settings_api.py — NO GIT CONFLICT but BROKEN

Same issue — remove local client() fixtures.