diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..c74411c --- /dev/null +++ b/.env.example @@ -0,0 +1,11 @@ +# Build-time, non-secret. Cloud origin used by the "Dewee Cloud" profile. +WXT_DEWEE_CLOUD_ORIGIN=https://app.dewee.sh + +# Live Playwright scenarios only (never baked into the build). Use a personal, +# owner-bound API key with operator.read + operator.write — never the master token. +E2E_GATEWAY_URL=http://localhost:18790 +E2E_GATEWAY_API_KEY= +E2E_WORKSPACE_ID= +# Live revocation scenario only, local test gateway: the key owner and the gateway admin token. +E2E_OWNER_ID= +E2E_GATEWAY_ADMIN_TOKEN= diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..1ead9d7 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,61 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + check: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm lint + - run: pnpm typecheck + - run: pnpm test + - run: pnpm build + - run: pnpm zip + - uses: actions/upload-artifact@v4 + with: + name: dewee-chrome-ext + path: .output/*.zip + # .output is a dot directory, which upload-artifact skips unless told otherwise. + include-hidden-files: true + if-no-files-found: error + + e2e: + runs-on: ubuntu-latest + needs: check + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm exec playwright install --with-deps chromium + - run: pnpm test:e2e + - uses: actions/upload-artifact@v4 + if: always() + with: + name: playwright-report + path: | + playwright-report/ + test-results/ + if-no-files-found: ignore diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..d8668fd --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,56 @@ +name: Release + +on: + push: + tags: ['v*'] + +permissions: + contents: read + +jobs: + build: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + # Dependency install scripts run in this job, so it gets no write token and keeps none in .git. + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm lint && pnpm typecheck && pnpm test + - name: Check tag matches package version + run: test "v$(node -p "require('./package.json').version")" = "${GITHUB_REF_NAME}" + # Unpacked-install zip (pinned extension ID) and Chrome Web Store zip (no manifest key). + - run: pnpm zip && pnpm zip:store + - name: Collect release files + run: | + mkdir release + cp .output/*.zip release/ + for f in .output-store/*.zip; do cp "$f" "release/$(basename "${f%.zip}")-store.zip"; done + cp CHANGELOG.md release/ + - uses: actions/upload-artifact@v4 + with: + name: release + path: release/ + retention-days: 7 + + publish: + needs: build + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: write + steps: + - uses: actions/download-artifact@v4 + with: + name: release + path: release + - name: Publish GitHub release + env: + GH_TOKEN: ${{ github.token }} + run: gh release create "$GITHUB_REF_NAME" release/*.zip --repo "$GITHUB_REPOSITORY" --title "$GITHUB_REF_NAME" --notes-file release/CHANGELOG.md diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..a9a9569 --- /dev/null +++ b/.gitignore @@ -0,0 +1,13 @@ +node_modules/ +.output/ +.output-e2e/ +.output-store/ +.wxt/ +dist/ +coverage/ +test-results/ +playwright-report/ +*.log +.env +.env.* +!.env.example diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..300f5ff --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,65 @@ +# AGENTS.md + +Guidance for humans and coding agents contributing to `nextlevelbuilder/dewee-chrome-ext`. + +## What this repo is + +A thin Chrome Manifest V3 client for Dewee. It renders a right side panel for chat with Dewee agents (Super Agent pinned first) and renders server-defined credential request cards. The Dewee Gateway is authoritative for agents, tenants, sessions, permissions and the credential store. Backend changes belong in [`nextlevelbuilder/dewee`](https://github.com/nextlevelbuilder/dewee); never copy Dewee source into this repo. + +Read before changing code: [docs/architecture.md](docs/architecture.md), [docs/api-contract.md](docs/api-contract.md), [docs/auth-threat-model.md](docs/auth-threat-model.md), [docs/credential-threat-model.md](docs/credential-threat-model.md), [docs/development.md](docs/development.md). + +## Commands + +```bash +pnpm install # also runs `wxt prepare` +pnpm dev # Chrome with the extension loaded and hot reload +pnpm lint +pnpm typecheck +pnpm test # unit + integration (Vitest) +pnpm test:e2e # builds, then runs Playwright against the unpacked build +pnpm build # .output/chrome-mv3 +pnpm zip # sideload zip (keeps the pinned extension key) +pnpm zip:store # Chrome Web Store zip (manifest without `key`) +``` + +Run `pnpm lint && pnpm typecheck && pnpm test && pnpm build` before every push. CI runs the same gates plus e2e. + +## Issue and PR workflow (required for every contributor) + +Status labels show who is working on what so contributors do not step on each other. + +| Label | When to set it | +|---|---| +| `in progress` | You started work. Assign yourself at the same time. | +| `blocked` | Work cannot continue (missing backend API, decision, credential). Comment with the blocker and a link. | +| `in review` | Implementation is done and an isolated reviewer (human or a subagent with fresh context) is reviewing. | +| `done` | The PR is open and linked with `Closes #N` / `Refs #N`. | +| `shipped` | The PR is merged into `main`. | + +Rules: + +1. Before starting, check the issue is not already `in progress` with another assignee. If it is, coordinate on the issue instead of starting parallel work. +2. Move one label at a time (remove the previous status label when you add the next). +3. Code review runs in an isolated context (a separate reviewer or subagent that did not write the code). Address blocking findings before `done`. +4. If you find an unrelated bug, gap or hardening need, open a new issue (labels `bug` or `security` plus `ai-handle` when an agent can take it) instead of widening the current PR. +5. Backend dependencies go to `nextlevelbuilder/dewee` as linked issues/PRs. Do not ship UI-only mocks for missing backend APIs. +6. Conventional commits, no secrets, no AI attribution in commit subjects. + +## Security rules (non-negotiable) + +- Never put secrets in chat messages, `chat.send`, URLs, logs, telemetry, Chrome `storage.local`/`sync`, or app-wide React state. Secret values live only in the credential form's local state and go only to the action respond endpoint. +- Never use the Dewee gateway master token as a client credential. Self-hosted mode uses a personal scoped API key (`operator.read`/`operator.write`). +- Never render agent-generated HTML. Cards render from a fixed list of kinds validated with Zod; markdown is sanitized with DOMPurify. +- No remote code, no `eval`, strict extension CSP. Host permissions are requested per gateway origin at runtime, never ``. +- The side panel never reads the active page. + +## Code layout + +- `src/entrypoints/` thin WXT entrypoints (background, sidepanel, options). +- `src/background/` service worker logic; `src/sidepanel/` and `src/options/` React UI. +- `src/api/` one typed `DeweeClient` for both modes (self-hosted Gateway and Cloud BFF) plus response schemas. +- `src/protocol/` Zod schemas for SSE events, UI actions and the background⇄panel messages. +- `src/auth/`, `src/chat/`, `src/credentials/`, `src/shared/` feature modules. +- `tests/unit`, `tests/integration`, `tests/e2e`. + +Keep files small and focused; kebab-case for new non-component files, PascalCase for React components. diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..eb17cb2 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,12 @@ +# Changelog + +## 0.1.0 — unreleased + +First MVP of the Dewee Chrome extension. + +- Side panel chat with Dewee agents: streaming, stop, retry, sessions, history paging, new chat, and reconcile after a lost stream without re-sending. +- Two connection modes: Dewee Cloud (OAuth Authorization Code + PKCE) and self-hosted Gateway (personal scoped API key). +- Workspace picker and searchable agent picker with Super Agent pinned first. +- Cards: secure credentials form for `secret_request`, `oauth_request`, choice, command preview, progress; other kinds shown read-only. +- Pending-card badge, English and Vietnamese, light/dark/system theme, keyboard shortcuts (Alt+Shift+D, Alt+N, /, Esc). +- Credentials kept in `storage.session` by default; opt-in "remember". diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..e072693 --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 Next Level Builder + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md index 18d9f89..4c18799 100644 --- a/README.md +++ b/README.md @@ -1,2 +1,47 @@ -# dewee-chrome-ext -Dewee Chrome Extension: secure multi-workspace agent chat, Super Agent and credential requests in a Chrome side panel +# Dewee Chrome Extension + +Chat with your Dewee agents, with Super Agent pinned first, in Chrome's side panel. When an agent needs an API key or an OAuth connection, the extension shows a secure card, so you never paste secrets into chat. + +- **Two connection modes:** Dewee Cloud (sign in with OAuth + PKCE) or a self-hosted Dewee Gateway (personal API key with `operator.read` + `operator.write`). +- **Workspaces and agents:** pick a workspace, search agents, and keep separate sessions per agent. +- **Reliable chat:** streaming, stop, retry, history, new chat. If the connection drops, the panel checks the server instead of sending your message twice. +- **Credential cards:** secrets stay in the form and go only to Dewee's respond endpoint. Nothing secret is stored by the extension. +- **Pending badge**, English and Vietnamese, light/dark/system theme, keyboard shortcuts. + +## Install (development build) + +```bash +pnpm install +pnpm build +``` + +Open `chrome://extensions`, enable **Developer mode**, choose **Load unpacked** and select `.output/chrome-mv3`. Pin the extension and click it (or press `Alt+Shift+D`) to open the side panel. + +## Connect + +- **Dewee Cloud:** choose *Dewee Cloud* and *Sign in*. You approve access on Dewee, then return to the panel. +- **Self-hosted:** choose *Self-hosted*, enter your Gateway URL (HTTPS, or `http://localhost` for development) and a personal API key created in Dewee. The Gateway operator must allow the origin `chrome-extension://poicgajeapmbcilfbeobdfedibiffbbm` (`GOCLAW_ALLOWED_ORIGINS`). Never use the gateway master token. + +By default the credential is kept only until Chrome closes. Tick *Remember on this device* to keep it on disk. + +## Keyboard shortcuts + +| Keys | Action | +|---|---| +| `Alt+Shift+D` | Open the side panel (change in `chrome://extensions/shortcuts`) | +| `Enter` / `Shift+Enter` | Send / new line | +| `Esc` | Stop the reply | +| `Alt+N` | New chat | +| `/` | Focus the message box | + +## Docs + +- [Architecture](docs/architecture.md) +- [Extension API contract](docs/api-contract.md) +- [Authentication threat model](docs/auth-threat-model.md) +- [Credential request threat model](docs/credential-threat-model.md) +- [Development](docs/development.md) and [contributor workflow](AGENTS.md) + +## License + +[MIT](LICENSE) diff --git a/docs/09-security.md b/docs/09-security.md new file mode 100644 index 0000000..e69de29 diff --git a/docs/api-contract.md b/docs/api-contract.md new file mode 100644 index 0000000..341e748 --- /dev/null +++ b/docs/api-contract.md @@ -0,0 +1,208 @@ +# Dewee Extension API contract (v1) + +The extension talks to Dewee through one versioned HTTP contract, served in two places: + +| Mode | Base URL | Credential | Served by | +|---|---|---|---| +| Self-hosted | `{gatewayOrigin}/v1/extension` | Personal API key, owner-bound and tenant-bound, scopes `operator.read` + `operator.write` | Dewee Gateway (Go) | +| Dewee Cloud | `{cloudOrigin}/api/extension/v1` | OAuth access token from the Extension OAuth client (Authorization Code + PKCE) | CCP app (`apps/dewee-app`), which calls the Gateway through the existing signed provisioning routes | + +Both bases expose the same paths, bodies and stream frames, so the extension has one typed client (`src/api/dewee-client.ts`) with two auth adapters. The Gateway stays authoritative for agents, sessions, runs, UI actions, RBAC and the encrypted credential store; the extension never decides permissions. + +Source of truth for this contract: this file plus the Zod schemas in `src/protocol/`. Backend changes live in `nextlevelbuilder/dewee` (see "Backend status" at the end of this file). + +## Conventions + +- JSON bodies use `snake_case`. Requests carry `Authorization: Bearer ` and `Content-Type: application/json`. Cookies are never used or accepted by these routes. +- Errors use one envelope and an HTTP status: + + ```json + { "error": { "code": "forbidden", "message": "Human-readable text", "retry_after_seconds": 30, "details": {} } } + ``` + + | Status | `code` examples | Client behavior | + |---|---|---| + | 400 | `invalid_request` | Show message; do not retry. | + | 401 | `unauthenticated`, `token_expired`, `token_revoked` | Cloud: one refresh attempt, then sign out. Self-hosted: mark profile as needing a new key. Never fall back to another profile or workspace. | + | 403 | `forbidden`, `owner_binding_required`, `scope_required`, `setup_human_actor_required`, `membership_suspended` | Show the reason; keep the session; no retry. | + | 404 | `not_found` (also for a workspace the caller is not a member of) | Show "not found or no access". | + | 409 | `action_closed`, `action_in_progress`, `response_conflict`, `rotate_required`, `run_in_progress`, `continuation_sent`, `continuation_in_progress`, `conflict` | Reload the action or run state from the server. Clients treat any 409 this way, including codes not listed. | + | 429 | `rate_limited` | Wait `retry_after_seconds` (or `Retry-After`). | + | 5xx | `runtime_unavailable` | Backoff and retry reads; never replay a chat send or action respond automatically. | + +- The Gateway rate limits writes (`chat/stream`, `actions/{id}/respond`) per API key; `chat/abort` is never limited, so a run can always be stopped. A 429 shows the wait time and leaves retry to the person. +- A workspace `viewer` can read (status, agents, sessions, history, actions) but gets 403 `forbidden` on stream, abort and respond, whatever the key's scopes. A run uses the lower of the tenant role and the key's role. +- Error messages never echo submitted values. +- `Cache-Control: no-store` on every response. +- CORS: only exact `chrome-extension://` origins configured by the operator are reflected; `Access-Control-Allow-Credentials` is never sent. + +## Endpoints + +### `GET /me` + +```json +{ "user": { "id": "u_123", "email": "a@b.c", "display_name": "An" }, + "auth": { "mode": "api_key", "scopes": ["operator.read", "operator.write"], "expires_at": null } } +``` + +`mode` is `api_key` (self-hosted) or `oauth` (Cloud). + +### `GET /workspaces` + +```json +{ "workspaces": [ { "id": "ws_1", "name": "Acme", "role": "owner", "status": "active" } ] } +``` + +- Cloud: every workspace where the user has a membership. `status` is `active`, `suspended` or `not_ready`; the panel lists the others but only opens `active` ones. +- Self-hosted: exactly one entry, the key's tenant (`id` = tenant UUID). +- `role` is `owner`, `admin`, `member` (Cloud) or `owner`, `admin`, `operator`, `viewer` (Gateway). The UI only uses it for hints; the server enforces. + +All paths below are under `/workspaces/{workspace_id}`. A workspace the caller cannot access returns 404. + +### `GET /workspaces/{id}/agents` + +```json +{ "agents": [ { "key": "super-agent", "id": "uuid", "name": "Super Agent", "status": "active", "model": "gpt-5", "provider": "openai", "is_super": true } ], + "super_agent_key": "super-agent" } +``` + +Only active agents the caller may chat with. `is_super` is true for `agent_key == "super-agent"` or the workspace's onboarding Super Agent id. The client pins it first. + +### `GET /workspaces/{id}/sessions?agent_key=&limit=20&offset=0` + +```json +{ "sessions": [ { "key": "agent:super-agent:app:direct:u_123-1a2b3c4d", "agent_key": "super-agent", "title": "…", "updated_at": "2026-10-11T10:00:00Z", "message_count": 12 } ], + "total": 1, "limit": 20, "offset": 0 } +``` + +Only the caller's own `app` channel sessions. `limit` is 1–50. + +### `POST /workspaces/{id}/chat/history` + +Request `{ "session_key": "…", "cursor": "…", "limit": 50 }` (cursor and limit optional). + +```json +{ "session_key": "…", "cursor": "…", "next_cursor": "…", "reset": false, + "messages": [ { "id": "m1", "role": "user", "content": "Hi", "created_at": "…" }, + { "id": "m2", "role": "assistant", "content": "Hello", "created_at": "…" } ] } +``` + +History is a follow log. `cursor` is a message position (`0` = oldest, default `0`), `limit` is 1–200 (default 50), and `next_cursor` is the position after the last message returned. A page shorter than `limit` means the end. `reset: true` means the cursor is past the end, and `next_cursor` is then the log length; a client that was following reads again. The cursor is at most 9 digits. The panel first asks with a cursor past the end to learn the length, follows only the newest 1,000 messages, and reveals them 50 at a time. + +Roles are `user`, `assistant` or `tool`; ids are `m`. System messages and Gateway setup-turn markers are never returned, and tool output is redacted. Turns the server wrote itself are returned only with `injected: true` (below). A session that does not belong to the caller returns 404. + +A message carries `injected: true` when the server, not the model or the person, wrote it. Since [dewee#1045](https://github.com/nextlevelbuilder/dewee/pull/1045) a run stopped before replying stores an injected assistant turn with the marker content `[Stopped before replying]`. The panel shows an injected assistant turn as an empty reply labelled "Stopped", like a live Stop, and never shows the marker text; it drops any other injected message, and never counts an injected user turn as one the person sent. + +### `POST /workspaces/{id}/chat/stream` + +Request: + +```json +{ "agent_key": "super-agent", "message": "Hello", "session_key": "optional", "setup_action_id": "optional uuid" } +``` + +- `session_key` absent starts a new server-generated session; the key is returned in `chat.started`. +- `setup_action_id` sends the Gateway-composed continuation of a resolved card instead of `message` (which must then be empty). The client never writes the continuation text itself. +- A `message` starting with `[[dewee:` is rejected (400). + +Response: `text/event-stream`, one frame per event, terminated by `message.completed`, `chat.error` or `chat.stopped`: + +| Event | Data | +|---|---| +| `chat.started` | `{ "run_id", "session_key", "agent_key" }` | +| `message.delta` / `thinking.delta` | `{ "delta" }` | +| `tool.call.started` | `{ "tool_call": { "id", "name", "arguments", "via_agent"? } }` | +| `tool.call.completed` / `tool.call.error` | `{ "tool_call": { "id", "name", "result_summary", "error"?, "via_agent"? }, "ui_action"?: }` | +| `reasoning.activity` | `{ "phase", "details" }` | +| `message.completed` | `{ "message": { "role": "assistant", "content", "created_at" }, "usage"?, "media"? }` | +| `chat.error` | `{ "code", "message", "recovery_action", "retry_after_seconds"? }` | +| `chat.stopped` | `{}` | + +These are exactly the frames the Gateway already emits for CCP (`internal/http/provisioning_chat_stream.go`). `ui_action` is present only after the Gateway re-validated the card; the client validates again (`src/protocol/ui-action.ts`). + +### `POST /workspaces/{id}/chat/abort` + +Request `{ "session_key": "…", "run_id": "optional", "agent_key": "optional" }` → `{ "aborted": true }`. + +### `GET /workspaces/{id}/sessions/{session_key}/status` + +`{ "running": true, "run_id": "…" }`. Used after the panel reopens or the service worker restarts, to reconcile instead of re-sending. + +### `GET /workspaces/{id}/actions?session_key=&status=pending` + +`{ "actions": [ ] }`. Reloads cards that were emitted while the panel was closed. + +### `POST /workspaces/{id}/actions/{action_id}/respond` + +Request (only these keys): + +```json +{ "response": { "fields": { "name": "openai-prod" } }, "secret": { "api_key": "" } } +``` + +- `secret` is accepted only for `secret_request` cards and only for declared `secret` fields (1–8 keys, each ≤ 16 KiB). `response` is ≤ 8 KiB. +- The caller must be a person (Gateway human-actor gate) with owner/admin on the workspace (Cloud) or owner/admin tenant role (self-hosted); otherwise 403 `setup_human_actor_required` or `forbidden`. + +Response: + +```json +{ "action": , "replayed": false, + "result": { "target_type": "provider", "name": "openai", "connected": true, "credential_ref": "cred:v1:provider:" }, + "continuation": { "session_key": "…", "agent_key": "super-agent", "setup_action_id": "" } } +``` + +The response never contains the secret. When `continuation` is present the client starts `chat/stream` with `setup_action_id`. The Gateway delivers a continuation at most once, so a client may resend a pending one, by hand or automatically, when it cannot tell whether the first one arrived: it is not a user message and cannot start a second run. A continuation counts as delivered only on `message.completed`, a 409 `continuation_sent` or a `chat.error` `setup_result_delivered`. `chat.started` is not proof: it is emitted before the first model call, and if the run fails before the result is saved (provider error, rate limit, exhausted credit) the Gateway releases the send lease, so the continuation can be sent again. The outcomes that settle a continuation (backend change [dewee#1047](https://github.com/nextlevelbuilder/dewee/issues/1047), implemented in [dewee#1022](https://github.com/nextlevelbuilder/dewee/pull/1022)): + +| Code | Meaning | Panel | +|---|---|---| +| `continuation_sent` (409) | The continuation was delivered. | Drops the item silently (no error bubble) and reloads the conversation. | +| `setup_result_delivered` (`chat.error`, `recovery_action` `continue_in_chat`) | The continuation reached the agent's session, but the run then failed. It must not be resent. | Drops the item, shows the error's message and lets the person continue in the chat. | +| `continuation_in_progress` (409) | Another request holds a live send lease (renewed every 2.5 min, stale after 10 min). It may still fail and release, so delivery is not confirmed. The response carries a `Retry-After` header and `error.retry_after_seconds` (currently 5). | Keeps the item and sends it again after the server's hint (5 s without one), then 15 s, 30 s and every 60 s, while its session is shown and idle, for about 11 min after the first such answer; then shows "Continue". | + +A 400, 404 or 410, or a 403 other than `scope_required` / `owner_binding_required`, means the card or continuation is invalid, gone or not allowed: the panel drops the item, says once that the card's result could not be delivered so the person should ask the agent again in chat (`continuation_refused`), and logs a `console.warn` with the status and code only. A 403 `scope_required` or `owner_binding_required` is a key problem the person can fix, so the item is kept, held, with "Continue" and the key error. Anything else is not proof of delivery: any other `chat.error`, a stream that closes early or drops after the response, Stop, or any other refusal, including an unknown 409 code. The panel keeps the item, held, and shows "Continue"; a held item is not sent automatically again. + +## Cloud authentication (Extension OAuth client) + +Public client `dewee-chrome-extension`, Authorization Code + PKCE S256, launched with `chrome.identity.launchWebAuthFlow`. + +1. `GET {cloudOrigin}/oauth/extension/authorize?response_type=code&client_id=dewee-chrome-extension&redirect_uri=https://.chromiumapp.org/oauth2&code_challenge=&code_challenge_method=S256&state=&scope=chat%20workspaces%20actions` + - Signs the person in with the existing web login if needed, shows a consent screen, then redirects to `redirect_uri?code=…&state=…`. + - `redirect_uri` must exactly match an allowlisted URI (env `DEWEE_EXTENSION_REDIRECT_URIS`). +2. `POST /api/extension/v1/auth/exchange` `{ "grant_type": "authorization_code", "code", "code_verifier", "redirect_uri", "client_id" }` + → `{ "access_token", "token_type": "Bearer", "expires_in": 900, "refresh_token", "refresh_expires_in", "scope" }`. Codes are single-use and expire after 60 s. +3. `POST /api/extension/v1/auth/refresh` `{ "refresh_token", "client_id" }` → same shape with a rotated refresh token. Reusing a rotated refresh token revokes the whole token family (replay detection). +4. `POST /api/extension/v1/auth/revoke` `{ "token" }` → `{ "revoked": true }`. Revokes the family. + +Tokens are opaque, stored hashed, bound to user, client and audience `dewee-extension`. They are not web sessions and are never accepted by cookie routes; Google/GitHub tokens are never used as Dewee bearers. + +## Generic agent credential requests + +Agents without `setup_operator` may emit only `secret_request` / `oauth_request` cards through the `credential_request` tool, and only when a workspace owner/admin granted the agent a policy: + +```json +{ "enabled": true, "targets": [ { "type": "provider", "provider_type": "openai", "modes": ["create", "update"] } ] } +``` + +The card uses the same `dewee.ui-action.v1` envelope, store, appliers, human-actor gate and continuation as setup cards. Holding the capability never allows reading, approving or submitting a credential. Policy is managed through the Gateway HTTP API, WS RPC and the `dewee agents credential-policy` CLI, by tenant owners/admins only. + +Tool-issued cards expire after one hour. The Gateway re-checks the agent's current policy when the person responds; a revoked or narrowed policy cancels the card and the respond call returns `409` with the card's usual error `code` plus `error.details.reason: "credential_policy_revoked"` (dewee#1019). The panel maps that reason to its localized `error.credential_policy_revoked` message; any other or malformed `details` keeps the envelope's own `code`. Like every 409 on respond, the panel reloads the card (now cancelled) and shows the reason. Repeat requests for the same target in a session return the existing open card, and open cards are capped per conversation and per agent. + +## Backend status + +Already on Dewee `dev`: runtime SSE frames including `ui_action`, the UI action store with respond, appliers, human-actor gate and continuation, `GET /v1/auth/token/status`, and owner-bound API keys. + +The extension depends on these open Dewee pull requests. They target `dev`; stacked PRs are retargeted after their base merges. + +| Capability | Pull request | Notes | +|---|---|---| +| Workspace chat runtime: deltas, run registration for status/abort, busy guard, one terminal frame, serialized SSE writes | [dewee#1021](https://github.com/nextlevelbuilder/dewee/pull/1021) | Closes #991 and #1001 | +| Gateway `/v1/extension/*` for personal API keys (streaming, abort, status, actions, CORS), tenant-scoped run lookups, a separate respond rate limit, `continuation_in_progress` vs `continuation_sent` | [dewee#1022](https://github.com/nextlevelbuilder/dewee/pull/1022) | Closes #956, #1036 and #1047; stacked on #1021 | +| `credential_request` capability for non-setup agents, the kept `credential_policy_revoked` reason, and policy writes refused without a human actor | [dewee#1019](https://github.com/nextlevelbuilder/dewee/pull/1019) | Closes #958 and #1035 | +| First-turn session visible to its owner during the run; a stopped run keeps the user turn in history, followed by an injected `[Stopped before replying]` marker | [dewee#1045](https://github.com/nextlevelbuilder/dewee/pull/1045) | Closes #1033 and #1034 | +| Extension OAuth client (PKCE, rotation, revoke) and CCP BFF `/api/extension/v1/*` | [dewee#1014](https://github.com/nextlevelbuilder/dewee/pull/1014) | Closes #957 | +| Own-session filter in the Gateway for the BFF sessions list | [dewee#1015](https://github.com/nextlevelbuilder/dewee/pull/1015) | Closes #992; stacked on #1014 | +| 160 KiB respond body cap on the browser route | [dewee#1011](https://github.com/nextlevelbuilder/dewee/pull/1011) | Closes #993 | +| Tenant-scoped run lookups for status and abort | [dewee#1010](https://github.com/nextlevelbuilder/dewee/pull/1010) | Closes #1000 | +| Key the router session-run index by tenant | [dewee#1028](https://github.com/nextlevelbuilder/dewee/pull/1028) | Closes #1008; stacked on #1021 | +| Scope run-ID abort, scheduler session queues and activity by tenant | [dewee#1031](https://github.com/nextlevelbuilder/dewee/pull/1031) | Closes #1029; stacked on #1028 | +| Scope the heartbeat busy check and throttle estimate by tenant; clear only the finishing run's activity | [dewee#1040](https://github.com/nextlevelbuilder/dewee/pull/1040) | Closes #1032; stacked on #1031 | diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..b48cd0e --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,54 @@ +# Architecture + +The extension is a thin Chrome MV3 client. Dewee (Gateway, or the CCP app for Dewee Cloud) owns agents, sessions, runs, permissions, card state and the credential store. The extension renders what the server returns and sends what the person types; it never decides permissions and never stores secrets. + +## Runtime pieces + +```text +┌──────────────── Chrome ────────────────┐ +│ Side panel (React) │ ┌──────────────────────────┐ +│ App → ChatShell → MessageList/Composer │ fetch │ Self-hosted Gateway │ +│ use-chat ── DeweeClient ───────────────┼───────►│ {gw}/v1/extension/* │ +│ CredentialsForm (secret in local state)│ └──────────────────────────┘ +│ │ runtime msgs (no card secrets) │ ┌──────────────────────────┐ +│ ▼ │ fetch │ Dewee Cloud (CCP BFF) │ +│ Background service worker ├───────►│ {cloud}/api/extension/v1 │ +│ token-manager (OAuth refresh, revoke) │ │ /oauth/extension/* │ +│ pending-badge (chrome.alarms, 1 min) │ └──────────────────────────┘ +│ Options page (profiles, theme, language)│ +└─────────────────────────────────────────┘ +``` + +| Module | Responsibility | +|---|---| +| `src/api/dewee-client.ts` | One typed client for both modes: JSON calls, SSE `chat/stream` with idle-stall detection, 401 → one renew and retry, `credentials: 'omit'`, `cache: 'no-store'`, `redirect: 'error'`. | +| `src/api/schemas.ts`, `src/protocol/*` | Zod schemas for every response, SSE frame, `dewee.ui-action.v1` card and background message. Unknown or invalid cards are dropped, never rendered. | +| `src/auth/credential-store.ts` | Credential per profile in `storage.session` (default) or `storage.local` (only when the person ticks "remember"). | +| `src/auth/cloud-oauth.ts`, `src/auth/pkce.ts` | Authorization Code + PKCE S256 through `chrome.identity.launchWebAuthFlow`. | +| `src/background/token-manager.ts` | Owns Cloud token renewal (single flight so a rotated refresh token is never replayed), sign in, sign out with revoke. | +| `src/background/pending-badge.ts` | Counts open cards for the active workspace and shows the number on the action badge. | +| `src/chat/use-chat.ts`, `chat-state.ts` | Chat engine: stream, stop, history paging, reconcile after a lost stream, card continuations. | +| `src/credentials/*` | Card rendering, the secure credentials form, labels from the extension's own copy. | +| `src/shared/settings.ts` | Non-secret settings: profiles, selections (profile → workspace → agent → session), theme, language. | + +## Data flows + +**Self-hosted connect.** The options page or the panel asks for host permission for exactly the gateway origin, calls `GET /me` with the API key, and only then stores the profile and credential. + +**Cloud connect.** The panel stores the profile, asks the background to run the OAuth flow, then calls `GET /me`. A failed first sign-in removes the new profile. + +**Chat.** `use-chat` posts `chat/stream`. `chat.started` gives the session key, which is remembered per workspace and agent. The panel never re-sends a message by itself, and it offers "Send again" only when the server provably did not receive the message: (a) the panel failed before calling fetch (background or credentials unavailable), (b) the fetch was rejected while `navigator.onLine` was false, or (c) the server refused the turn with an explicit 4xx before accepting it (validation, auth, rate limit, or a busy 409 such as `run_in_progress`). Every other failure is uncertain: a 5xx (including a proxy's 502, 503 or 504), a stall, a drop after the response, a fetch rejected while online, a decoder fault, a history read that failed, or a turn not found after a reconcile. An uncertain turn stays visible once, marked `delivery_unconfirmed`, with "Reload conversation" (or "Show conversations" in a new chat, which has no session to check). It never gets "Send again"; the text stays on screen, so the person can copy it and send it again by hand. After an uncertain failure in a known session, or a break after the server answered (200, any frame or `chat.started`), the engine polls run status and reloads history; polling pauses while the browser is offline, slows down after about 3 minutes and keeps the composer closed until the run ends. Only one reconcile runs per conversation, and a switch, unmount or newer reconcile retires the old one. An unconfirmed turn is dropped only when the history provably holds it: the message that was last at send time (its `m` id, role and text) is still in place, and a user turn with the same text follows it (past the turns confirmed by `chat.started` since). After compaction, renumbered ids, or a highest position below the send-time one, the anchor is gone and the turn stays unconfirmed. A history that cannot be read (including one that keeps shrinking while it is read) is never treated as empty: the transcript stays and the turn stays unconfirmed. Unconfirmed turns survive later history reloads and new sends, and every reconcile or reload checks them again. Residual risks: the offline check is a heuristic, so if the connection drops after the request bytes left, `onLine` can already be false and "Send again" may start a second run; and an identical text sent again after the anchor can make a lost repeat look delivered. Stop calls `chat/abort` and aborts the fetch; pressed before `chat.started` names the run, the abort is sent as soon as it does (or the fetch is dropped after 5 s). + +**Cards.** `ui_action` frames and `GET /actions?status=pending` feed the card list. A response goes to `POST /actions/{id}/respond`. If the response is lost (network error or 409) the panel reloads the card. A resolved card's `continuation` is queued for its session, tagged with the profile and workspace captured when the respond call started (a switch during the call does not re-tag it). Queued continuations are sent one at a time once their own session is shown and idle, so two cards answered during a reply both continue after it, and a continuation never streams into another conversation. The queue lives in `storage.session` with one key per item (profile, workspace and setup action; the value holds only those IDs plus session and agent, a `held` flag and the time of the first in-progress answer, never a secret). Adding or removing an item writes only that key, so panels in two windows never remove each other's items, and each panel follows `storage.onChanged` to see what another panel added or delivered. A session or agent switch or a closed panel does not lose an item; reopening the session sends what is pending. Both behave the same while a continuation streams: the stream is dropped without holding the item (this is not a Stop), so it is sent again when its session is shown, here or in a reopened panel, and the Gateway answers `continuation_sent` if it was delivered meanwhile. Unlike user messages, continuations are sent automatically, because the Gateway delivers each at most once. An item leaves the queue only when the agent provably got it: a `message.completed` frame, a 409 `continuation_sent` (already delivered; a silent success that reloads the conversation, with no error bubble), or a `chat.error` with code `setup_result_delivered` (the result reached the agent's session but the run failed; its message is shown and the person continues in the chat). Earlier frames such as `chat.started` are not proof: if the run fails before the result is saved (provider error, rate limit, exhausted credit), the Gateway releases the send lease. It also leaves when it can never succeed (400, 404 or 410, or a 403 other than a key problem: the card or continuation is invalid, gone or not allowed); the panel then says once, with no "Continue", that the card's result could not be delivered and the person should ask the agent again in chat (`continuation_refused`), and logs a `console.warn` with the status and code only (a 400 is a client bug). A 403 `scope_required` or `owner_binding_required` can be fixed by the person, so that item is held with "Continue" and the key error. A 409 `continuation_in_progress` means another request holds a live send lease that may still fail and release, so it is not delivery: the item stays and is sent again after the server's `Retry-After` hint (5 s without one), then 15 s, 30 s and every 60 s, only while its session is shown and idle, for about 11 minutes after the first such answer (a lease goes stale after 10 minutes; the start is stored with the item, so reopening a panel does not restart the window); then it shows "Continue". Any other outcome keeps the item, held, and shows "Continue": any other `chat.error`, `chat.stopped` or Stop, a stream that closes without a terminal frame or drops after the response (the reconcile offers "Continue" once the run ends), and any other refusal, including an unknown 409 code. Resending is safe: the Gateway answers `continuation_sent` if it was delivered and `continuation_in_progress` while a run holds the lease. A held item is stored with a `held` flag, so it is not sent automatically again, neither after a conversation switch nor in a reopened panel; when its session is shown, it offers "Continue". The stored flag wins in every panel: a held copy stored by another panel stops this panel's automatic retries, and an unheld copy (another panel pressed "Continue") releases the item here and removes this panel's "Continue". When another panel delivers the item, its "Continue" here goes away too, and a click on a stale one does nothing. Remaining limit: a held item (including one past the in-progress retry window) stays in `storage.session` until the person presses "Continue" or the browser restarts. + +**Restart.** Reopening the panel or a service worker restart loads history, run status and pending cards for the remembered session. A user message is never sent automatically. Only pending card continuations are sent automatically (see Cards), because the Gateway delivers each at most once. + +## Decisions + +- The panel calls the API directly. Moving every call through the service worker would add a message hop per SSE frame and a second place where tokens live. Only OAuth refresh, which must be single flight across pages, runs in the background. +- Secrets never cross `chrome.runtime` messaging. The background message schema has no secret fields. +- Card field labels and target names come from the extension's own copy. Agent text is shown as plain text, so an agent cannot dress up a phishing form. +- Host permissions are optional and requested per origin at connect time. There is no `` and no content script. +- One workspace per view. A load error in one workspace never falls back to another workspace or profile. + +See [api-contract.md](api-contract.md) for the wire contract and the two threat models for security reasoning. diff --git a/docs/auth-threat-model.md b/docs/auth-threat-model.md new file mode 100644 index 0000000..5ae555e --- /dev/null +++ b/docs/auth-threat-model.md @@ -0,0 +1,37 @@ +# Authentication threat model + +Scope: how the extension proves who the person is to Dewee, and how those credentials are kept. + +## Assets + +| Asset | Where it lives | Lifetime | +|---|---|---| +| Cloud access token (opaque, audience `dewee-extension`) | `storage.session`, or `storage.local` if "remember" | 15 min | +| Cloud refresh token (rotating, family revocation on reuse) | same as access token | server-defined | +| Self-hosted personal API key (`operator.read` + `operator.write`, owner-bound) | same rule | until revoked in Dewee | +| PKCE verifier and `state` | memory of the background worker during one flow | one flow | + +The Dewee gateway master token is never accepted as an extension credential, and the UI asks for a personal key only. + +## Threats and controls + +| Threat | Control | +|---|---| +| Authorization code interception | PKCE S256; exact redirect URI `https://.chromiumapp.org/oauth2`; codes single-use, 60 s expiry. | +| CSRF / mix-up on the OAuth redirect | Random `state` compared in constant time; redirect origin and path checked before reading `code`. | +| Refresh token replay | Rotation on every refresh; a reused token revokes the family. The background runs renewals single flight so the extension itself never replays a rotated token. | +| Token theft from disk | Default storage is `storage.session` (memory, cleared when the browser closes). "Remember" is opt-in and its hint explains the trade-off. | +| Stolen token used from a web page | Routes accept bearer tokens only, never cookies; CORS reflects only configured `chrome-extension://` origins and never sends `Access-Control-Allow-Credentials`. | +| Token sent to the wrong host | Each profile has one origin; requests use `credentials: 'omit'`, `redirect: 'error'`; host permission is granted per origin. HTTPS is required except for loopback development. | +| Origin with embedded credentials (`https://user:pass@host`) | Rejected by `checkOrigin`. | +| Wrong workspace or tenant | Every call names the workspace; Dewee enforces membership and returns 404 for non-members. The client never falls back to another workspace or profile. | +| Expired or revoked credential | 401 → one renew (Cloud) → on failure the credential is cleared and the panel asks to reconnect that same profile. | +| Sign-out leaves a live token | Sign-out calls revoke (best effort) and clears both storage areas. Removing a profile also drops its host permission when no other profile uses the origin. | +| Messages from other extensions or pages | The background accepts messages only from its own extension id and validates them with Zod. It returns the `Authorization` value only to those pages, which hold it in memory for the request. A network failure while renewing keeps the credential; only a 401 signs the profile out. | +| Remote code | Strict CSP (`script-src 'self'`), no `eval`, no remote scripts. | + +## Residual risks + +- Malware with access to the Chrome profile can read `storage.session` while the browser runs. Tokens are short-lived and revocable. +- "Remember" keeps the credential on disk in the Chrome profile; the person accepts this when they tick it. +- Self-hosted operators must allowlist the extension origin (`GOCLAW_ALLOWED_ORIGINS`) and keep API keys scoped. diff --git a/docs/credential-threat-model.md b/docs/credential-threat-model.md new file mode 100644 index 0000000..a424dd0 --- /dev/null +++ b/docs/credential-threat-model.md @@ -0,0 +1,33 @@ +# Credential request threat model + +Scope: how a person gives a secret (API key, password, token) to Dewee when an agent asks for one through a `secret_request` card, and how OAuth connections are started through an `oauth_request` card. + +## Flow + +1. An agent with `setup_operator`, or an agent granted a `credential_request` policy, emits a `dewee.ui-action.v1` card. The Gateway stores it. +2. The panel receives the card from the stream or from `GET /actions?status=pending`, validates it with Zod and renders `CredentialsForm`. +3. The person types the secret. The value lives only in the form component's local state. +4. Submit sends `{ response: { fields }, secret: { : } }` to `POST /actions/{id}/respond` and clears the inputs before awaiting the result. +5. The Gateway checks the human-actor gate and role, encrypts the secret into its credential store, and returns the card status, a non-secret `result` and an optional `continuation`. The agent continues with a credential reference, never the value. + +## Threats and controls + +| Threat | Control | +|---|---| +| Secret typed into chat | The form is the only secret input. Chat text is never inspected or rewritten; docs and the form tell people to use the card. | +| Secret persisted by the extension | No storage call carries a secret; secret fields are `type=password` with `autocomplete=new-password`. Tests check `storage.local`/`session` and runtime messages after a submit. | +| Secret in logs, URLs or telemetry | No telemetry. Secrets go only in a JSON body to one endpoint. Errors from the server never echo submitted values. | +| Secret passed between extension pages | The background message schema has no secret fields; the form calls the API directly. | +| Phishing card (agent pretends to be another service) | Field labels and destination names come from the extension's copy keyed by declared field and target type. Agent-provided text is plain text. The card shows the destination workspace. | +| Agent asks for a secret it should not have | Only `setup_operator` agents or agents with an owner-granted `credential_request` policy can create cards, and only for policy targets. Holding the policy never allows reading or approving a credential. | +| Automation or another agent submits a secret | Gateway human-actor gate: only a person with owner/admin rights can respond. | +| Duplicate submit or lost response | Respond is idempotent per card; the panel reloads the card after a network error or 409 and continues only through `setup_action_id`, delivered at most once. | +| Pending continuations kept across a switch or a closed panel | The panel stores only IDs in `storage.session` (profile id, workspace id, `session_key`, `agent_key`, `setup_action_id`), a boolean `held` flag and `in_progress_since` (a millisecond timestamp of the first 409 `continuation_in_progress`). These are not secrets and never include a card response or secret value. | +| Overwriting an existing credential by accident | Update mode needs an explicit rotate step. | +| Malicious OAuth link in a card | The panel shows the third-party notice and opens Dewee; completion is checked server side (`oauth_not_completed` until it is). | +| Rendering attacks (HTML/JS in card or markdown) | No agent HTML. Markdown is sanitized with DOMPurify, images dropped, only http(s)/mailto links, `rel="noopener noreferrer nofollow"`. | + +## Residual risks + +- A compromised browser or OS can read keystrokes; out of scope for the extension. +- The person can still paste a secret into chat on purpose. Dewee-side redaction is the backstop. diff --git a/docs/development.md b/docs/development.md new file mode 100644 index 0000000..2c6f372 --- /dev/null +++ b/docs/development.md @@ -0,0 +1,75 @@ +# Development + +## Requirements + +- Node 22+, pnpm 10 (`corepack enable`). +- Chrome 116+ (side panel API). +- For end-to-end work: a Dewee Gateway (self-hosted mode) or a CCP app (Cloud mode). See [api-contract.md](api-contract.md). + +## Setup + +```bash +pnpm install +cp .env.example .env # optional; only build-time, non-secret values +pnpm dev # opens Chrome with the extension and hot reload +``` + +`.env` keys (all optional): + +| Key | Purpose | +|---|---| +| `WXT_DEWEE_CLOUD_ORIGIN` | Cloud origin baked into the build. Default `https://app.dewee.sh`. | +| `E2E_GATEWAY_URL`, `E2E_GATEWAY_API_KEY`, `E2E_WORKSPACE_ID` | Used only by the live Playwright scenarios. Never baked into the build. | +| `E2E_OWNER_ID`, `E2E_GATEWAY_ADMIN_TOKEN` | Live tests only, on a local test gateway: mint and revoke a throwaway key, set and delete a credential-request policy. | +| `E2E_GATEWAY_LOG`, `E2E_DB_TOOL`, `E2E_DB_DSN` | Optional live credential checks: the secret is absent from the Gateway log and stored encrypted. | +| `E2E_CLOUD_ORIGIN`, `E2E_CLOUD_EMAIL`, `E2E_CLOUD_PASSWORD` | Live Cloud scenario only, against a local Dewee app. Test values; never a real account. | +| `E2E_ALLOW_REMOTE` | The live specs skip when `E2E_GATEWAY_URL` or `E2E_CLOUD_ORIGIN` is not loopback (`localhost`, `127.0.0.1`, `[::1]`). Set to `1` only to target a remote throwaway test stack on purpose. | + +The unpacked extension id is pinned by the public key in `extension-public-key.json`: `poicgajeapmbcilfbeobdfedibiffbbm`. Register this redirect URI with the CCP OAuth client: `https://poicgajeapmbcilfbeobdfedibiffbbm.chromiumapp.org/oauth2`. + +## Local gateway for self-hosted mode + +1. Start the Dewee Gateway with `GOCLAW_ALLOWED_ORIGINS=chrome-extension://poicgajeapmbcilfbeobdfedibiffbbm` so the extension origin passes CORS. +2. In Dewee, create a personal API key bound to your user with scopes `operator.read` and `operator.write`. Do not use the gateway master token. +3. In the extension, choose **Self-hosted**, enter `http://localhost:` (HTTP is accepted only for loopback) and the key. + +## Quality gates + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +pnpm test:e2e # builds, then Playwright loads .output/chrome-mv3 +pnpm zip # .output/dewee-chrome-ext--chrome.zip (keeps the pinned key, for sideloading) +pnpm zip:store # .output-store/… without `key`, for the Chrome Web Store +``` + +The Chrome Web Store rejects a manifest that carries `key` and assigns its own extension ID. Register that store ID's `chromiumapp.org` redirect URI and `chrome-extension://` origin next to the unpacked ones. + +### Live end-to-end + +The `tests/e2e/live-*.spec.ts` specs drive the built extension against a real local Dewee Gateway: connect, agents, chat, stop, key revocation, reopening the panel, reconciling a lost response (`live-selfhosted`), stream resilience (`live-resilience`), credential cards and admin policy (`live-credentials`), and Cloud sign-in through a local Dewee app (`live-cloud`). Each is skipped unless its environment is set (shell or `.env`). The model is the deterministic stub `tests/e2e/support/stub-llm.mjs`; `tests/e2e/support/setup-live-gateway.mjs` creates the tenant, agents and personal key and writes them to `.env`. Steps: [tests/e2e/support/live-gateway.md](../tests/e2e/support/live-gateway.md). + +```bash +pnpm build:e2e +npx playwright test tests/e2e/live- # all live specs; screenshots in test-results/live/ +``` + +CI (`.github/workflows/ci.yml`) runs the same gates and uploads the built extension and Playwright report. Tagging `v*` runs `.github/workflows/release.yml`, which attaches the zip to a GitHub release. + +## Contract sync + +`scripts/sync-dewee-contract.mjs` copies the `dewee.ui-action.v1` schema and examples from a Dewee checkout into `tests/contract-fixtures/ui-action-v1`. Contract tests fail when the Zod schemas drift from Dewee. + +## Conventions + +- Strict TypeScript, Zod at every trust boundary, React function components. +- kebab-case for modules, PascalCase for components. +- No secrets in storage, messages, logs or URLs (see [credential-threat-model.md](credential-threat-model.md)). +- Follow the issue label workflow in [AGENTS.md](../AGENTS.md). + +## Decision record + +**ADR-001: panel calls the API directly; background owns OAuth renewal.** Streaming through the service worker would add a hop per frame and a second token holder. Renewal must be single flight across pages because refresh tokens rotate, so it lives in the background. The worker hands the current `Authorization` value only to this extension's own pages (sender id checked); card secrets never cross runtime messaging. + +**ADR-002: credentials in `storage.session` by default.** Memory-only storage limits exposure; "remember" (opt-in, with a hint) uses `storage.local` for people who accept the trade-off. + +**ADR-003: one versioned Extension API for both modes.** Self-hosted `/v1/extension` and Cloud `/api/extension/v1` share paths and bodies so there is one client and one set of schemas. diff --git a/docs/journals/261012-dewee-chrome-ext-bootstrap.md b/docs/journals/261012-dewee-chrome-ext-bootstrap.md new file mode 100644 index 0000000..3931f10 --- /dev/null +++ b/docs/journals/261012-dewee-chrome-ext-bootstrap.md @@ -0,0 +1,58 @@ +# Dewee Chrome extension bootstrap: retry policy and continuations + +**Date**: 2026-10-12 (Asia/Saigon) +**Severity**: Medium +**Component**: side panel chat (`use-chat.ts`), card continuations, Dewee Gateway extension API +**Status**: Resolved. Shipped as PR #3 (Closes #1) with 12 dependent Dewee PRs. Follow-up #2 (SSE buffer cap) is in progress. + +## What Happened + +We bootstrapped the MV3 side panel: chat, sessions and credential cards. Two areas ate most of the review budget: deciding when to offer "Send again", and making sure a card's continuation is never lost. Final evidence: 21/21 live e2e tests passed in two runs in a row, and 376 unit and integration tests pass. + +## The Brutal Truth + +We kept writing smarter heuristics, and the reviews kept breaking them. That was humbling. Every "delivered" proof we built on history ids failed, because the server rewrites those ids. Continuations needed three review rounds. Each round found a new way to lose the agent's card result silently. That is the worst failure for a setup flow: the agent just waits. + +## Technical Details + +- **Retry.** History ids are `m` indices. `TruncateHistory(keepLast)` renumbers them after compaction, so a delivered turn at baseline 140 came back as `m3`. The panel then showed it twice, with "Send again" (review X-1). +- **Continuation queue.** + - A one-slot `pendingContinuationRef` overwrote the first of two answered cards (review X-2). + - It is now a per-id queue in `chrome.storage.session`, with one key per item and no read-modify-write. +- **Ambiguous 409.** The panel could not tell a live send lease from a finished delivery. Dewee now returns `continuation_in_progress` and `continuation_sent` separately (dewee#1047). +- **`chat.started` is not delivery.** The Gateway calls `lease.release()` when the run fails before the result is saved (provider error, rate limit), so we dequeued too early (review B-1). +- **Backend bugs found by the live e2e.** + - #1022 dropped the 409 details, so test D showed a generic message. Fixed in 2d02e47a. + - #1045 merged a stopped turn into the next message. Fixed with an injected `[Stopped before replying]` marker. + +## What We Tried + +1. A position baseline. Compaction broke it. +2. An anchor heuristic plus a "match anywhere" fallback. A null baseline made a lost "yes" look delivered. +3. Dequeue on the first frame or on a clean close. This lost results whenever the lease was released. + +## Root Cause Analysis + +We tried to infer delivery from client-side signals the server never promised to keep stable. The real fix was a policy, not cleverer code: +- Offer "Send again" only when the server provably did not get the turn: no fetch was made, the fetch failed while offline, or the server sent an explicit 4xx refusal. +- Treat everything else as uncertain. Show "Delivery could not be confirmed" with a reload action. +- Continuations leave the queue only on `message.completed`, `continuation_sent` or `setup_result_delivered`. Any other outcome holds the item with "Continue". + +## Lessons Learned + +- If an id comes from a list index, it is not identity. Ask the backend for an explicit outcome code before writing heuristics. +- Use fresh-context reviewers for at-most-once and exactly-once logic. Each round caught a real data-loss path. +- Live e2e found two backend bugs that unit tests could not see. + +## Environment Pain + +- The Bash tool failed with `ENAMETOOLONG`, so reviewers read code only and never ran gates. Some subagent types have no PowerShell. +- Some Go tests already failed on Windows before this work. +- `gw-env.ps1` lacked the provisioning key pair. Later runs used the launcher `gw-start4.ps1`. + +## Open Risks and Next Steps + +- **59.5 s e2e outlier.** It did not reproduce, and it still fits the in-progress retry ladder (5/15/30 s). Capture a trace if it happens again. +- **Refused and held continuations.** These paths (400/403/404/410 and "Continue") are tested only by unit tests. Add a live scenario. +- **Real Dewee merge.** Regenerate `spec.hash.lock`, and fix the `recordingChatAborter` abort test fake that `go vet` flags. +- **Follow-up #2.** Finish the SSE buffer cap. diff --git a/docs/workspace-setup-onboarding.md b/docs/workspace-setup-onboarding.md new file mode 100644 index 0000000..e69de29 diff --git a/eslint.config.js b/eslint.config.js new file mode 100644 index 0000000..0c6c686 --- /dev/null +++ b/eslint.config.js @@ -0,0 +1,32 @@ +import js from '@eslint/js'; +import tseslint from 'typescript-eslint'; +import reactHooks from 'eslint-plugin-react-hooks'; +import globals from 'globals'; + +export default tseslint.config( + { ignores: ['.output/', '.output-e2e/', '.output-store/', '.wxt/', 'node_modules/', 'coverage/', 'test-results/', 'playwright-report/'] }, + js.configs.recommended, + ...tseslint.configs.recommended, + { + files: ['**/*.{ts,tsx}'], + languageOptions: { globals: { ...globals.browser, ...globals.node, chrome: 'readonly' } }, + plugins: { 'react-hooks': reactHooks }, + rules: { + ...reactHooks.configs.recommended.rules, + 'no-eval': 'error', + 'no-implied-eval': 'error', + 'no-new-func': 'error', + 'no-console': ['error', { allow: ['warn', 'error'] }], + '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }], + }, + }, + { + // Playwright fixtures call `use()`, which is not React's hook. + files: ['tests/e2e/**/*.ts'], + rules: { 'react-hooks/rules-of-hooks': 'off' }, + }, + { + files: ['scripts/**/*.{js,mjs}', 'tests/e2e/support/**/*.mjs'], + languageOptions: { globals: { ...globals.node } }, + }, +); diff --git a/extension-public-key.json b/extension-public-key.json new file mode 100644 index 0000000..1d41efb --- /dev/null +++ b/extension-public-key.json @@ -0,0 +1,4 @@ +{ + "id": "poicgajeapmbcilfbeobdfedibiffbbm", + "key": "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAl1IGEAWHjb7MJznCoe+jgdSCl1UWDdxF2+bK+RwV3Ldk2hWigxT8AYrtzDBHyKd7Hrv9liQjdEOelqwDu1ScjjxAUhWXwa/gFc1rOqk+iay8eb/B8vSrqvJRnLdMIXUZ3qNvzTMY7IvZNnRp2mTPrYK32RlzI+GgiMjldqN8Oq1ZoMcIltzw7Q4ZNCK2Fm5V+2oOq7xvPYhxQynPpkfX4+LTgAP8k9iYX7vk6N7k/LYb5/pFFNLJ/V/+Y3xdCBuGI3rJgE3A0gESR152WH+NsXSkwxD0tS7Fr1JGuWyarFfnzOGOKRfIpci65AftUhekn8fjoITLK0m0Z/Ma8tADVwIDAQAB" +} diff --git a/package.json b/package.json new file mode 100644 index 0000000..6ce5b01 --- /dev/null +++ b/package.json @@ -0,0 +1,51 @@ +{ + "name": "dewee-chrome-ext", + "version": "0.1.0", + "private": true, + "description": "Dewee Chrome Extension: secure multi-workspace agent chat, Super Agent and credential requests in a Chrome side panel", + "license": "MIT", + "type": "module", + "packageManager": "pnpm@10.32.1", + "engines": { + "node": ">=22" + }, + "scripts": { + "dev": "wxt", + "build": "wxt build", + "zip": "wxt zip", + "zip:store": "node scripts/zip-store.mjs", + "postinstall": "wxt prepare", + "lint": "eslint .", + "typecheck": "tsc --noEmit", + "test": "vitest run", + "test:watch": "vitest", + "build:e2e": "node scripts/build-e2e.mjs", + "test:e2e": "node scripts/build-e2e.mjs && playwright test" + }, + "dependencies": { + "dompurify": "3.4.16", + "marked": "18.1.0", + "react": "19.3.0", + "react-dom": "19.3.0", + "zod": "4.6.5" + }, + "devDependencies": { + "@eslint/js": "^10.0.1", + "@playwright/test": "1.64.0", + "@testing-library/react": "^16.3.3", + "@testing-library/user-event": "^14.6.7", + "@types/chrome": "0.3.5", + "@types/node": "^24.19.2", + "@types/react": "^19.3.0", + "@types/react-dom": "^19.3.0", + "@wxt-dev/module-react": "1.2.2", + "eslint": "10.12.0", + "eslint-plugin-react-hooks": "^7.1.1", + "globals": "^17.13.0", + "jsdom": "^30.1.2", + "typescript": "5.9.3", + "typescript-eslint": "8.71.1", + "vitest": "5.0.3", + "wxt": "0.21.4" + } +} diff --git a/plans/reports/assets/live-e2e/a-cloud-reply.png b/plans/reports/assets/live-e2e/a-cloud-reply.png new file mode 100644 index 0000000..3cd4b1a Binary files /dev/null and b/plans/reports/assets/live-e2e/a-cloud-reply.png differ diff --git a/plans/reports/assets/live-e2e/b2-streamed-reply.png b/plans/reports/assets/live-e2e/b2-streamed-reply.png new file mode 100644 index 0000000..eeca084 Binary files /dev/null and b/plans/reports/assets/live-e2e/b2-streamed-reply.png differ diff --git a/plans/reports/assets/live-e2e/b3-stopped.png b/plans/reports/assets/live-e2e/b3-stopped.png new file mode 100644 index 0000000..d623900 Binary files /dev/null and b/plans/reports/assets/live-e2e/b3-stopped.png differ diff --git a/plans/reports/assets/live-e2e/b4-revoked.png b/plans/reports/assets/live-e2e/b4-revoked.png new file mode 100644 index 0000000..0d75778 Binary files /dev/null and b/plans/reports/assets/live-e2e/b4-revoked.png differ diff --git a/plans/reports/assets/live-e2e/c-card-done.png b/plans/reports/assets/live-e2e/c-card-done.png new file mode 100644 index 0000000..2166935 Binary files /dev/null and b/plans/reports/assets/live-e2e/c-card-done.png differ diff --git a/plans/reports/assets/live-e2e/c-card-pending.png b/plans/reports/assets/live-e2e/c-card-pending.png new file mode 100644 index 0000000..d4973eb Binary files /dev/null and b/plans/reports/assets/live-e2e/c-card-pending.png differ diff --git a/plans/reports/assets/live-e2e/d-revoked-refused.png b/plans/reports/assets/live-e2e/d-revoked-refused.png new file mode 100644 index 0000000..9ac189d Binary files /dev/null and b/plans/reports/assets/live-e2e/d-revoked-refused.png differ diff --git a/plans/reports/assets/live-e2e/e-lost-respond-reconciled.png b/plans/reports/assets/live-e2e/e-lost-respond-reconciled.png new file mode 100644 index 0000000..26b1900 Binary files /dev/null and b/plans/reports/assets/live-e2e/e-lost-respond-reconciled.png differ diff --git a/plans/reports/assets/live-e2e/e-offline.png b/plans/reports/assets/live-e2e/e-offline.png new file mode 100644 index 0000000..b45d26f Binary files /dev/null and b/plans/reports/assets/live-e2e/e-offline.png differ diff --git a/plans/reports/assets/live-e2e/e-online-reconciled.png b/plans/reports/assets/live-e2e/e-online-reconciled.png new file mode 100644 index 0000000..bae3e68 Binary files /dev/null and b/plans/reports/assets/live-e2e/e-online-reconciled.png differ diff --git a/plans/reports/assets/live-e2e/e1-reconciled.png b/plans/reports/assets/live-e2e/e1-reconciled.png new file mode 100644 index 0000000..c676d07 Binary files /dev/null and b/plans/reports/assets/live-e2e/e1-reconciled.png differ diff --git a/plans/reports/assets/live-e2e/e1-unconfirmed.png b/plans/reports/assets/live-e2e/e1-unconfirmed.png new file mode 100644 index 0000000..2c39d55 Binary files /dev/null and b/plans/reports/assets/live-e2e/e1-unconfirmed.png differ diff --git a/plans/reports/code-reviewer-261011-1910-extension-review.md b/plans/reports/code-reviewer-261011-1910-extension-review.md new file mode 100644 index 0000000..98f2cd1 --- /dev/null +++ b/plans/reports/code-reviewer-261011-1910-extension-review.md @@ -0,0 +1,94 @@ +# Code review: Dewee Chrome MV3 extension (bootstrap) + +Scope: src/**, wxt.config.ts, scripts/*, .github/workflows/*, docs/* (tests/e2e/live-* and tests/e2e/support/* excluded). +Method: full read of every src file against AGENTS.md + the four docs; upstream Dewee Go source checked for contract types. +Gates: NOT run. The Bash tool fails on every command in this session and no PowerShell tool was exposed, so lint/typecheck/vitest results are unverified. + +## Blocking + +### B1. Every settings write tears down the chat: the first reply of a new chat is never streamed +- `src/sidepanel/App.tsx:29` derives `profile = activeProfile(settings)`; `loadSettings()` re-parses storage, so every `update()` and every `storage.onChanged` refresh yields a new `profile` object. +- `src/sidepanel/ChatShell.tsx:49` `useMemo(() => createClient(...), [profile])` then builds a new `DeweeClient` on every settings change. +- Effects: (a) `use-directory.ts:39,60` keys requests on `client`, so workspaces/agents go back to `data: null, loading: true`; `workspaceId` becomes undefined; `useChat(null)` runs. (b) `use-chat.ts:116` `sameScope` compares `scope.client === ctx.client`, so it is false. Either way the effect bumps the epoch, aborts `streamRef`, and resets state. +- Concrete failure: user sends the first message in a new chat. `chat.started` calls `onSessionKey` → `setSession` → `update()` (ChatShell.tsx:79). The settings change creates a new client, the live stream is aborted locally (AbortError swallowed by the epoch check), the panel flashes skeletons, and the reply only shows up later through reconcile/history. The same thing happens on every theme/language change in the options page, on the initial `withSelectedWorkspace` write (ChatShell.tsx:63), and on every agent/session switch. Each of those also re-fetches workspaces and agents. +- Not covered: no test mounts ChatShell, and e2e smoke has no chat flow. +- Fix: memoize on primitives, e.g. `useMemo(() => createClient(...), [profile.id, profile.kind, profile.origin])`, and add an integration test that a settings write during a stream does not abort it. + +## Should-fix + +### S1. Any background failure becomes a 401 and wipes the credential (local sign-out without revoke) +- `src/api/client-factory.ts:16` maps every `{ ok: false }` from the worker to `DeweeApiError(401, …)`. That includes `network_error`, `runtime_unavailable` and `timeout` from a proactive refresh (token-manager.ts:54, within 60 s of expiry), and `background_unavailable`/`no_response` from messaging. +- `App.tsx:36` `onAuthError` then runs `clearCredential`. +- Scenario: Wi-Fi blips while the access token is near expiry. The panel signs out and drops the refresh token without revoking it. This contradicts token-manager.ts:35 ("network errors do not end the session") and the contract (sign out only after a rejected refresh). +- Fix: keep the real status (use status 0 for transport/background failures). Clear only on `token_revoked`/`not_connected`/`api_key_rejected`, or let the worker own clearing. + +### S2. Single-flight refresh can still replay a rotated refresh token (family revoked) +- `src/background/token-manager.ts:51-54`: `authorizationFor` reads the credential, then awaits `profileOf()`, then calls `renew(…, credential.refreshToken)`. If the in-flight task finishes (write + `inflight.delete`) inside that gap, this caller starts a new refresh with the stale R1. The server sees reuse and revokes the family. +- `credential-store.ts:38-40` widens the window: with `remember`, the code writes local before it removes session, and `readCredential` prefers session, so a concurrent reader gets the old token. +- Likely trigger: at expiry, the panel fires about 6 parallel calls (workspaces, agents, sessions, history, status, actions) plus the badge alarm. +- Fix: check `inflight` before reading. Inside the locked task, re-read the credential and skip the network refresh if its `refreshToken` already differs from the one the caller saw (or if `accessExpiresAt` is fresh). Remove the stale area before writing the new one, or write session first. + +### S3. History cursor type does not match the Gateway follow log +- `api/schemas.ts:64-66` accepts only string `cursor`/`next_cursor`, and `chat/history.ts:19` sends `cursor: String(n)`. +- Dewee's existing follow-log handlers use integers: `dewee/internal/http/provisioning_chat_history.go:24,38-39` (`Cursor *int`, `NextCursor int`) and `sessions.go:221`. +- If the `/v1/extension` adapter reuses `newSessionHistoryFollowResponse`, two things break: every history call returns 400 (`"0"` into `*int`), and every response fails Zod with `invalid_response`. No conversation would load. +- The unit tests use strings, so they cannot catch this. +- Fix: accept `z.union([z.number().int(), z.string()])`, send a number, and state the JSON type in docs/api-contract.md. + +### S4. Sessions longer than 5,000 messages show a stale middle window, not the newest messages +- `chat/history.ts:9-10,18`: `MAX_PAGES = 25` × 200. When the loop stops at the page cap, `messages.slice(-1000)` holds positions 4000–4999, not the tail. +- Opening a long session also costs up to 25 sequential POSTs, and the reconcile path (`use-chat.ts:104`) repeats them. +- Fix: start the follow at `max(0, total - MAX_KEPT_MESSAGES)` (the Gateway returns `total`; add it to the schema), or drop the page cap. + +### S5. A card continuation is silently dropped while a run is streaming or reconciling +- `ChatShell.tsx:103` calls `chat.continueWith(result.continuation)`, but `use-chat.ts:133` returns early when the phase is `streaming` or `reconciling`. +- Cards arrive in `tool.call.completed` mid-stream, so the person can submit before `message.completed`. The respond succeeds, the continuation is never sent, and the agent never resumes. Nothing tells the user. +- Fix: queue the continuation and send it when the phase returns to idle, or show a "Continue" action. +- Related: `continueWith` ignores `continuation.agent_key` and uses the current agent (use-chat.ts:145). Pass it through. + +### S6. Stop before `chat.started` leaves an orphan server run +- `use-chat.ts:202-212`: in a new chat there is no `sessionKey` yet, so no `chat/abort` is sent and only the fetch is aborted. +- The server may already have accepted the turn. The run keeps going, and it can execute tool and setup actions, in a session the panel never learns about. The UI shows "Stopped". +- Fix: if Stop is pressed before `chat.started`, keep reading until `chat.started` arrives (or a short timeout), then call abort with the returned session and run. + +### S7. `loadFailed` has no recovery path; `reload()` is dead code +- `MessageList.tsx:100` shows "Could not load this conversation" with no retry button. `useChat().reload` (use-chat.ts:217) is never used. +- The user can only recover by switching session or agent. Wire `chat.reload` to a retry button. + +### S8. Release workflow hardening and a store-incompatible manifest +- `.github/workflows/release.yml`: the job runs with `contents: write`. `actions/checkout` keeps the token in `.git/config` by default, and `pnpm install` runs dependency `postinstall` scripts, which can read it. Fix: set `persist-credentials: false` on checkout, and move `contents: write` to a separate publish job (or step) that does not install dependencies. +- The release job skips e2e, and the zip is built after the tag check without `pnpm build` gating. That is acceptable, but CI on the tag commit is not enforced. +- `wxt.config.ts:17` always sets `manifest.key`, and the release zip is labelled "store-ready". As far as I know, the Chrome Web Store rejects uploads whose manifest has a `key` field. Please confirm. Fix: drop `key` unless building dev/e2e. + +### S9. The origin check accepts loopback hosts the manifest cannot grant +- `shared/origin.ts:8,11` accepts `http://[::1]` and `http://*.localhost`. +- `wxt.config.ts:23` `optional_host_permissions` only lists `http://localhost/*` and `http://127.0.0.1/*`. `permissions.request` rejects these hosts, and the user sees a misleading "Chrome did not grant access". +- Fix: add `http://[::1]/*` (and `http://*.localhost/*` if wanted) to the manifest, or narrow the check. + +## Nits +- N1 `SessionList.tsx:60` + `ChatShell.tsx:258`: the limit is capped at 50, but "Show more" still renders when `total > 50` and then does nothing. Use `offset` paging or hide the button. +- N2 `ChatShell.tsx:139-141`: Alt+N starts a new chat while streaming, even though the "New chat" button is disabled when `busy`. Apply the same guard to the shortcut. +- N3 `use-chat.ts:89-91`: if the stream closes without a terminal frame before `chat.started` (no session yet), the phase goes idle but the assistant bubble keeps `streaming: true`, so the typing indicator stays on forever. +- N4 `use-chat.ts:133`: the re-entrancy guard reads `stateRef`, which only updates after commit. Prefer `if (streamRef.current) return;` so a double invocation in the same tick cannot send twice. The Composer currently prevents this, but the hook should not rely on it. +- N5 AGENTS.md and architecture.md say secrets never cross runtime messaging, but `auth.authorization` returns the bearer API key/access token to pages over `runtime.sendMessage` (service-worker.ts:20). This is low risk because the sender id is checked and only own pages can call it, but either document the exception or let pages read `storage.session` directly and keep only renewal in the worker. +- N6 `CredentialsForm.tsx:26`: the destination label interpolates agent-supplied `target.name` (≤100 chars). This partly undermines "labels come from extension copy". Consider showing only the type plus `provider_type`. +- N7 `i18n.ts`: en/vi parity is enforced by `Record`, which is good. Client codes `conflict`, `profile_not_found`, `background_unavailable`, `oauth_invalid_redirect`, `oauth_missing_code`, `oauth_access_denied` and `internal_error` fall back to "Something went wrong (code)". Add copy, at least for `conflict` (409 is a documented path). +- N8 `connect.ts:45-46`: `connectGateway` writes the credential before `saveProfile`. If `saveSettings` throws (schema parse), an orphan credential stays in storage. Write the profile first, or clear the credential on failure. +- N9 `settings.ts:64` `updateSettings` is a non-atomic read-modify-write shared by the panel, the options page and the worker. Concurrent writes (options theme change plus panel session write) can lose an update. This is low impact, but worth a single serialized writer. + +## Verified OK (risk calibration) +- No `console`, `eval`, `innerHTML`, `localStorage` or `storage.sync` in src. CSP is `script-src 'self'`. There is no content script, no `tabs`/`scripting`/`activeTab`, and no ``. Host access is requested per origin inside the click handler before any await. +- Requests use `credentials: 'omit'`, `redirect: 'error'` and `no-store`. HTTPS is enforced except on loopback. URLs with userinfo are rejected. +- PKCE S256 uses a 32-byte verifier. `state` is compared in constant time, and the redirect origin/path is checked before reading `code`. Revoke is best effort and the credential is always cleared. +- Every response, SSE frame and card is Zod-validated. Card envelopes are strict, unknown kinds are dropped, and `target.url` is refused. Card text renders as plain text. Markdown goes through marked → DOMPurify with an allowlist; images are dropped; only http(s)/mailto links survive, with `noopener noreferrer nofollow`. +- Secret values stay in CredentialsForm local state, are cleared before the await, are sent only to `/respond`, and do not appear in `chat-state`. +- The worker rejects foreign `sender.id` and Zod-validates messages. +- A 409 on stream or respond triggers a reload or reconcile, never a re-send. Reopening the panel loads history, status and cards without sending anything. + +## Unresolved questions +1. Will the `/v1/extension` history adapter emit integer cursors, as the existing Gateway follow log does? This decides S3. +2. Should the store build carry `key`, or is it dev/e2e only? (S8) + +Status: DONE_WITH_CONCERNS +Summary: One blocking defect (settings writes rebuild the client and abort or reset live chats, including the first reply of every new chat), plus 9 should-fix issues around auth error mapping, a refresh-token replay race, history contract and paging, dropped continuations, and release hardening. +Concerns/Blockers: Bash was broken and no PowerShell tool was available, so pnpm lint/typecheck/test were not run. All findings come from reading the code. diff --git a/plans/reports/code-reviewer-261011-1930-dewee-956-957-review.md b/plans/reports/code-reviewer-261011-1930-dewee-956-957-review.md new file mode 100644 index 0000000..3b1a970 --- /dev/null +++ b/plans/reports/code-reviewer-261011-1930-dewee-956-957-review.md @@ -0,0 +1,146 @@ +# Review: Dewee 956 (Gateway /v1/extension) + 957 (CCP extension OAuth + BFF) + +Reviewer: isolated code-reviewer, read-only. Date: 2026-10-11. + +## Limits of this review +- Bash fails in this session (`unexpected EOF` from the profile) and no PowerShell tool was available, so + `git show`, `go test`, `go vet` and `vitest` were NOT run. The commit message, exact diff hunks and + the plan/PR parity matrix could not be checked. The review reads the worktree files directly + (assumes HEAD == 5e3e03ae / 14183ba5 with a clean tree). +- Files read: 956 `internal/http/extension_api{,_read,_chat,_actions}.go`, `extension_api_sqlite_test.go`, + `provisioning_chat_stream.go`, `provisioning_chat_setup_turn.go`, `workspace_setup_actor_gate.go`, + `workspace_setup_respond_core.go`, `workspace_setup_actions_routes.go`, `auth.go`, `api_keys.go`, + `gateway/server.go` (CORS), `cmd/gateway_http_wiring.go`. 957 `lib/extension-oauth/*`, + `lib/extension-api/*`, all `pages/api/extension/v1/**`, `pages/oauth/extension/authorize.astro`, + both migrations, `http-guards.ts`, `local-password-accounts.ts`, `workspace-session-read-scope.ts`. + +## Blocking + +**B1. CCP history returns tool output without redaction, and returns runtime-injected user turns** +`apps/dewee-app/src/lib/extension-api/extension-api-views.ts:102-113` +- The view keeps `role: "tool"` messages with raw `content`. It drops only user turns that start with `[[dewee:`. + The upstream provisioning history (`internal/http/provisioning_chat_history.go:126-141` -> + `sessions.go:228`) returns raw `providers.Message` and does not redact. +- Failure: a tool result that echoes an env var, a config file or a provider error with a key goes in + plaintext to the extension. The web chat skips tool messages (`workspace-chat-history.ts:27`), so this + route shows more than the web does. An injected runtime turn (not marker-prefixed) also appears as the + person's own text. +- The contract says "tool output is redacted" and "runtime-injected turns are never returned". The + Gateway path does both (`extension_api_read.go:339-355`, using `redactTracePreview` and `Injected`). +- Fix: redact tool content server-side with the same patterns, or drop tool messages. Expose the + `injected` flag through the provisioning history (or a filtered variant) and drop those turns. Add a + test with a secret-shaped tool result. + +**B2. A tenant `viewer` with a write-scoped key runs chat as `operator` (or `admin`)** +`internal/http/extension_api.go:113-119` (`runRole`), used at `extension_api_chat.go:115-117` +- `permissionRoleFromTenantRole("viewer")` returns `""` (`provisioning_chat_runner.go:94-105`). + `runRole` then returns `c.KeyRole`. For a key with `operator.write` that is `operator`; with + `operator.admin` it is `admin`. +- The `auth(write=true)` check looks only at key scopes, never at the tenant role. A viewer whose + system owner minted an owner-bound read+write key (`api_keys.go:114-149` allows this) can start runs, + and the tools in those runs act above the viewer's tenant role. The comment above `runRole` says the + run role is "never more than the key grants", but here the tenant role is what gets exceeded. +- Fix: map `viewer` to `RoleViewer`. Use the minimum of the tenant role and the key role. Refuse + `chat/stream` and `chat/abort` with 403 `forbidden` when the tenant role is below operator/member. + Add a test for a viewer key on `/chat/stream`. + +## Should-fix + +**S1. Account recovery does not revoke extension sign-ins** (957) +`src/lib/local-password-accounts.ts:136` deletes every web session on password recovery but leaves +`extension_oauth_token_families` alone. Result: someone who took over the account and approved the +extension keeps API access (refresh tokens rotate) for up to 90 days after the owner recovers it. +Fix: add `UPDATE extension_oauth_token_families SET revoked_at=?, revoked_reason='account_recovery' WHERE user_id=? AND revoked_at IS NULL` +to the same batch. Apply the same change to any other "sign out everywhere" path. + +**S2. The CCP respond body cap is below the contract maximum** (957) +`pages/api/extension/v1/workspaces/[workspaceId]/actions/[actionId]/respond.ts:28` uses the default 64 KiB cap +(`extension-api-route.ts:145`). The contract allows `response` up to 8 KiB plus 8 secrets of up to 16 KiB each +(about 136 KiB, which the Gateway's own route accepts: `extension_api_actions.go:21`). A valid multi-field +secret card is refused as "too large". Fix: pass `8*1024 + 8*16*1024 + framing`. + +**S3. The two servers return different history shapes** (956 vs 957) +Gateway ids are `m` (`extension_api_read.go:322`, matching the contract). CCP ids are bare numbers +(`extension-api-views.ts:108`). A limit above 200 is clamped by the Gateway (`extension_api_read.go:307`) +but rejected with 400 by CCP (`history.ts:75`). Pick one behavior: `m` + position, and either clamp or reject in both. + +**S4. No rate limit on `/v1/extension/*`** (956) +Nothing on the extension routes returns 429. `chat/stream` and `respond` can be called with no limit by a +leaked key. CCP limits every route (for example `stream.ts:40`). Fix: apply the server `RateLimiter` +keyed by API key id on the write routes and add `Retry-After`. Also add `Retry-After` to the CORS +expose list (`server.go:1392`). + +**S5. CCP session list scans the whole tenant and caps it** (957) +`pages/api/extension/v1/workspaces/[workspaceId]/sessions.ts:34-46`: up to 10 sequential runtime calls +per request (each list call fetches every app chat in the tenant). A member's chats older than the +newest 500 app chats in the tenant are never listed, and `total` is wrong. Fix: filter by user on the +gateway list (own_only-style) and page there. + +**S6. Risky paths without tests** (956) +`extension_api_sqlite_test.go` does not cover: a customer session token (`kind='session'`) being refused; +a key owner who is no longer a tenant member; a suspended tenant (`membership_suspended`); a viewer +streaming chat (B2). The fixture calls `RegisterRoutes` directly on a mux +(`extension_api_sqlite_test.go:109-110`), so CORS and preflight for a `chrome-extension://` origin +through `server.go` are not tested either. + +**S7. Pending cards beyond the newest 200 are dropped** (956) +`extension_api_actions.go:44-54` fetches one page (`SetupActionListMax` = 200), filters by status +afterwards, and never follows the cursor. Older pending cards in a busy session are silently left out. +Push the status filter into the store query, or keep paging. + +## Nits +- `extension_api_read.go:369-374`: `ownedSession` trims `session_key`, but `IsSessionBusy` and `SessionRunID` + get the untrimmed path value. Trim once and use that value everywhere. +- `extension_api_actions.go:134`: for a concierge card (`setup:concierge`) whose creator agent is not found, + `continuation.agent_key` is `""`. The extension's `continuation` schema needs `min(1)`, so the whole + respond result fails to parse after the card has already been answered. +- `extension-oauth-service.ts:141-147`: a code replay that lands between `consumeExtensionCode` and + `insertExtensionFamily` sees `family_id` NULL and revokes nothing. +- `extension-oauth-service.ts:201-207`: marking the old refresh token rotated and inserting the new pair + are separate batches. If the insert fails, the old token is spent and the person must sign in again. + Batch them together. +- `pages/api/extension/v1/workspaces.ts:14-27`: N+1, two reads per membership. +- `extension-chat-relay.ts:98-128`: if the runtime stream ends without a terminal frame, the relay closes + without sending `chat.stopped` or `chat.error`. The contract says every stream ends with one of them. +- Status `run_id`: CCP sends `null`, the Gateway omits the field. The client accepts both; the contract + should say which one is correct. +- `server.go:1381`: the gateway CORS allowlist accepts a `*` entry (this predates the commit, and the + extension routes inherit it). The contract says exact origins only. + +## Checked and found correct +- Gateway auth: the gateway token is refused, compared in constant time (`auth.go:38-43`). Agent-chat, + MCP-only, customer-scope, session and ownerless keys are refused. Tenant and owner come from the key; + `enrichContext` forces the user to the owner (`auth.go:555-564`). `{workspace_id}` must equal the key's + tenant, otherwise 404. Membership is re-checked on every request. Tenant-scope-only is forced. +- Gateway sessions: `isProvisionedWorkspaceChatSession` checks user, `app` channel, agent UUID and key. + A tenant check on the agent blocks cross-tenant access. Foreign-session 404s are tested. +- Respond and continuation: owner/admin plus the human gate with the key's lineage (`APIKeyID` in ctx). + Tenant-scoped load, a claim CAS, single-use continuation lease. Secret size and count are validated in + `uiaction/responses.go:51-65`. The response does not include the secret. `zeroSecrets` is deferred. +- Shared refactor: `handleProvisionWorkspaceChatStream` builds the same context and stream as before. + `setupActionChatTurn` keeps the provisioning path's `Human: true` and `CanWrite` checks. No regression + found for existing callers. +- CCP OAuth: PKCE S256 only, with strict verifier and challenge formats. The challenge comparison is + timing-safe. Codes are single-use through a conditional UPDATE, and a replay revokes the family. + Refresh rotation and replay revoke the family. The redirect URI must match an exact, + pattern-filtered allowlist. Only hashes are stored. A revoked family is re-checked while a stream runs. +- Consent: POST is guarded by an exact `Origin` match (`http-guards.ts:3-8`) and a per-user rate limit. + The page sends `frame-ancestors 'none'`, XFO DENY and no-store. `state` is bounded. +- CCP CORS and bearer: exact `chrome-extension://` origins only, a foreign `Origin` is refused, no + Allow-Credentials. The routes never read the cookie. Error messages for 5xx and unknown errors are generic. +- Migrations: D1 0034 and self-hosted 0033 have the same tables. The self-hosted one writes its schema-version row. + ISO-8601 TEXT timestamps compare correctly on both. The conditional UPDATEs rely on `rowsAffected`, + which the PG adapter maps (`statement-database.ts:102-103`). + +## Recommended order +1. B1, B2 (with tests). 2. S1, S2, S3. 3. S4, S5, S6, S7. 4. Nits. + +## Unresolved questions +- Should a tenant `viewer` be able to chat from the extension at all, or only read? (This decides the B2 fix.) +- With an empty `session_key`, `GET /actions` returns concierge cards to any member with read or chat access, on both + servers. Is that the intended reload scope for the panel? +- Does the 956 PR include the required trace/surface/CLI parity statements? Not checked, because git was unavailable. + +Status: DONE_WITH_CONCERNS +Summary: 2 blocking (CCP history leaks unredacted tool output and injected turns; Gateway viewer role runs chat as operator/admin) and 7 should-fix issues. The auth, PKCE, rotation, CORS, CSRF and migration code is otherwise sound. +Concerns/Blockers: Shell unavailable, so tests, vet and git show were not run. Findings come from reading the source. diff --git a/plans/reports/code-reviewer-261011-2140-dewee-956-957-rereview.md b/plans/reports/code-reviewer-261011-2140-dewee-956-957-rereview.md new file mode 100644 index 0000000..71ddd39 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2140-dewee-956-957-rereview.md @@ -0,0 +1,116 @@ +# Re-review: Dewee 956 fix a078e857 + 957 fix fcab6128 + +Reviewer: isolated code-reviewer, read-only. Date: 2026-10-11. Baseline: `code-reviewer-261011-1930-dewee-956-957-review.md`. + +## Limits +- Bash fails in this session (`unexpected EOF` from the profile), so `git show`, `go test` and `vitest` were NOT run. + I read the worktree files directly and assume HEAD equals a078e857 / fcab6128 with a clean tree. +- The implementers' test runs (go 13+5 extension tests, vitest 91 tests) were not reproduced. + +## Original findings + +| ID | Status | Evidence | +|---|---|---| +| B1 CCP history leak | **Closed** | `extension-api-views.ts:105-117` drops system/unknown roles, empty turns, `injected` user turns and `[[dewee:` turns, and runs tool `content` through `redactSensitiveString`. Only `id/role/content/created_at` are emitted, so `tool_calls`, `thinking` and `media_refs` never leave the server. `injected` is really serialized upstream (`providers/types.go:179`, `provisioning_chat_history.go:41`). Same rule as Gateway `extension_api_read.go:339-355`. Tests are at `extension-api-routes.test.ts:390,422`. | +| B2 viewer runs as operator/admin | **Closed** | `extension_api.go:271-280` refuses an unknown tenant role on every route and a viewer on stream/abort/respond. `runRole` (`:162-171`) is min(tenant, key) via `HasMinRole`. Tests are at `extension_api_sqlite_test.go:549,570`. | +| S1 recovery keeps extension sign-ins | **Closed** | `local-password-accounts.ts:138`: the revoke runs in the same `db.batch` as `DELETE FROM sessions`. | +| S2 respond cap 64 KiB | **Closed** | `respond.ts:23,31`: the 160 KiB cap applies to that route only. The default stays 64 KiB (`extension-api-route.ts:145`). The upstream provisioning body limit is 1 MiB (`provisioning.go:216`), so nothing is refused downstream. | +| S3 history shape parity | **Closed** | Ids are `m${cursor+index}` on both sides. Limit: <1 gets 400, >200 is clamped to 200 on both (`history.ts:74-80`, `extension_api_read.go:301-307`). | +| S4 no rate limit (956) | **Closed (scoped)** | `extension_api.go:97-109` wraps only stream and respond, after `auth`. The key is `extension_key:`, never the token (`:99`, test `:648`). Retry-After is exposed (`server.go:1392`). The limit is off when `gateway.rate_limit_rpm=0` (`gateway_http_wiring.go:449`). | +| S5 CCP sessions scan | **Partially closed** | `sessions.ts:44-54`: `total` is now honest for the window. Chats older than the tenant's newest 500 are still invisible, and a runtime filter is still needed (follow-up issue). | +| S6 missing tests | **Closed** | Session token, former member, suspended, viewer and CORS through `Server.Handler()` (`extension_cors_test.go:59`). | +| S7 pending cards beyond 200 | **Mostly closed** | `extension_api_actions.go:59-82` follows the cursor up to 10 pages (2000 cards). This is bounded, not complete. Acceptable. | +| Nit status trim | Closed | `extension_api_read.go:371-378`. | +| Nit empty continuation agent_key | Closed | `extension_api_actions.go:142-146` returns `continuation: null`. Test at `:714`. | +| Nit code replay before family row | **Closed** | `consumeExtensionCode` reserves `family_id` in the CAS UPDATE (`extension-oauth-store.ts:97`). The replay upserts a revoked family (`:145-154`). The winner's INSERT then hits the PK, `isExtensionFamilyRevoked` returns true, and the result is `invalid_grant` (`extension-oauth-service.ts:176-179`). The prune (`expires_at < now-1d`) cannot delete that row. | +| Nit refresh rotation two batches | **Closed** | `rotateExtensionRefreshToken` (`extension-oauth-store.ts:163-180`) runs as one batch. D1 `batch` is a transaction. The `SqlStatementDatabase.batch` adapter refuses a multi-statement batch without `transaction()` (`statement-database.ts:44-58`), and `rowsAffected` maps `changes`. On PG, the row lock on the conditional UPDATE serializes racers, so the loser sees 0 changes. The loser's tokens are written but never returned, and its family is revoked. | +| Nit relay without terminal frame | **Closed** | `extension-chat-relay.ts:137-140`. | +| Nit run_id null vs omitted | Closed (documented) | Both forms are documented. | +| Nit workspaces N+1 | Open (accepted) | Parallel reads. Low impact. | +| Nit CORS `*` entry | Open | Not addressed. Predates 956. | + +## New findings + +### Blocking + +**N1. 956: a detached extension run cannot be stopped and has no deadline** +`internal/http/extension_api_chat.go:120,160` +- `runCtx` is now `context.WithoutCancel(ctx)`, but the extension runner never registers the run with the router. + `agentRouterWorkspaceChatRunner.RunWorkspaceChat` (`provisioning_chat_runner.go:25-67`) calls `loop.Run` directly. + `RegisterRun` is only called by `methods/chat.go:454`, `chat_completions.go`, `responses.go`, `wake.go` and `webhooks_llm.go`. +- As a result, `AbortRunsForSession`/`AbortRun` find nothing, so `chat/abort` answers `aborted:false`. + `IsSessionBusy` is always false, so the `run_in_progress` guard (`:102`) never fires. +- The WS chat path pairs `WithoutCancel` with `WithCancel` + `RegisterRun` (`methods/chat.go:444-454`). The extension now has + neither a cancel func nor a router entry. +- I found no run-level deadline in `internal/agent` (no `WithTimeout`/`WithDeadline` around `loop.Run`). The report's claim + "the runtime's own run timeout still applies" (`fullstack-...-956-review-fixes.md:23`) is not supported by the code. +- Failure scenario: + - The person presses Stop, or closes the panel on a runaway tool loop. Before a078e857 the disconnect cancelled the run. + Now it keeps running and spending provider tokens until the loop ends by itself. + - The person reopens the panel and resends. A second concurrent run starts in the same session, because the busy check is blind. + - A leaked write key can start 20 unstoppable runs per minute (and unlimited runs when rpm=0). +- No goroutine is leaked: the handler stays blocked in `streamWorkspaceChat` until `loop.Run` returns. But the request + goroutine and its SSE writer now live as long as the run. +- The test `TestExtensionAPIStreamRunSurvivesClientDisconnect` uses a fake runtime (`e.runtime.running`), so it hides this. +- Fix (any one): + - (a) Revert the detach until #991 lands router registration. + - (b) Do it here: `runCtx, cancel := context.WithCancel(WithoutCancel(ctx))`, call `RegisterRun(runCtx, runID, sessionKey, agentID, cancel)` + with `defer UnregisterRun`, and add a hard `context.WithTimeout` cap. + - Add a test against a real `*agent.Router` that abort cancels the run ctx and that `IsSessionBusy` is true during the run. + +### Should-fix + +**N2. 956: the rate limit is silently off when `gateway.rate_limit_rpm=0`** (`cmd/gateway_http_wiring.go:449`) +- Operators who disable the WS limiter also lose the only throttle on extension stream and respond. +- Combined with N1, a leaked key has no bound at all. +- Fix: give the extension its own limiter with a non-zero floor, or document the coupling in `docs/18-http-api.md` section 38. + +**N3. 957: the relay can send two terminal frames** (`extension-chat-relay.ts:141-142`) +- The `catch` calls `writeError` without checking `terminated`. +- Scenario: `message.completed` is written, then `bestEffortMarkWorkspaceChatReady` or `touchExtensionChatRun` throws + synchronously (or any later await rejects outside a `.catch`). The client then gets `chat.error` after `message.completed`. +- Fix: `if (!terminated) writeError(error)`. + +**N4. 957: the sessions route fans out to up to 10 parallel runtime calls per request** +`sessions.ts:48` makes up to 10 parallel calls, and each call lists the tenant's app chats. With the 120/min read limit per +person, one member can drive about 1200 runtime list calls a minute. +- A chat updated between page reads can also be missed (it leaves its old page before that page is read). Dedupe does not help. +- Fix: lower the per-route limit, or cache page 0 for a few seconds. The real fix is still the runtime own-only filter. + +### Nit +- `extension_api.go:201-204`: the `membership_suspended` branch in `auth` is now dead (`resolveCaller` returns first). Remove it, + or keep it with a comment. +- `extension_api.go:256`: `ErrTenantEntitlementInactive` probably covers every non-active entitlement, not only `suspended`. A + pending or cancelled workspace would then also say "suspended". Confirm against `store` and map only `suspended`. +- `created_at` parity: the Gateway formats RFC3339 seconds and omits a nil timestamp. CCP passes the runtime string, or `""` + (`extension-api-views.ts:128`). The client should accept both, or one side should change. +- Refresh loser path: the losing rotation writes a pair under the family before revoking it. If `revokeExtensionFamily` then + fails, a live, never-disclosed pair remains (hash only). Harmless, but `rotated_at` could be gated inside the inserts with + `WHERE changes()`-style SQL. + +## Specific checks requested +- **WithoutCancel**: no goroutine leak (the call is synchronous), but the run outlives the client with no deadline and no abort. See N1. +- **Rate limit order and key**: after auth; the key is `extension_key:`, not the token. It is a single shared bucket + for stream+respond per key, with abort and reads excluded on purpose. +- **Redaction coverage**: only `content` is emitted for tool messages, and it is redacted. Assistant `content` is not redacted on + either server (same as the web chat), so a model that quotes a secret from a tool result still shows it. This matches the + contract text ("tool output is redacted") and is noted as residual risk, not a defect. +- **Batch atomicity**: real on D1 (implicit transaction), on the APP_SQL adapter (requires `transaction()`), and in the test helper (`BEGIN`). +- **160 KiB scope**: the respond route only. Every other extension route keeps 64 KiB. +- **Forbidden shortcuts**: none found. + - The Origin/CSRF consent guard is intact (`extension-oauth-authorize.ts:116`, `http-guards.ts:3-8`). + - The gateway token is still refused (`extension_api.go:225`). + - No `setup_operator` widening, and no secret in chat (the respond view is built field by field, and `zeroSecrets` is deferred). + +## Recommended actions +1. N1 before merge of 956: revert the detach, or add a cancel func, router registration and a hard timeout, with a test. +2. N3 (one line), N2 (doc or own limiter). +3. N4 and the S5 runtime filter as a linked follow-up issue in nextlevelbuilder/dewee. + +## Unresolved questions +- Is #991 (router registration) going to merge before #956? If so, N1 drops to Should-fix, provided #991 also adds a run deadline. +- Should the extension limiter be independent of `gateway.rate_limit_rpm`? + +Status: DONE_WITH_CONCERNS +Summary: Every original blocking and should-fix finding is closed or bounded, except S5, which is partial. The detach in 956 adds a blocking regression: extension runs cannot be aborted, have no deadline, and the busy guard is blind. +Concerns/Blockers: N1 blocks 956. Shell unavailable, so tests and diffs were not run, and the findings come from reading the source. diff --git a/plans/reports/code-reviewer-261011-2140-dewee-958-review.md b/plans/reports/code-reviewer-261011-2140-dewee-958-review.md new file mode 100644 index 0000000..c019cae --- /dev/null +++ b/plans/reports/code-reviewer-261011-2140-dewee-958-review.md @@ -0,0 +1,117 @@ +# Security review: nextlevelbuilder/dewee#958 (credential_request capability) + +Branch `feat/958-credential-request-capability` (c486f99c, a7ddbe71, 13795e4d on bd177a2d). I am an isolated reviewer and did not write this code. Paths below are relative to `D:/www/nextlevelbuilder/worktrees/dewee-958-credential-request-capability`. I verified each finding by reading the source; I ran no commands. + +## Verdict +**Not ready to merge.** The policy surfaces themselves are sound: +- The policy cannot be written through a generic agent update or an import. +- Writes require an admin and a person; an agent credential is refused. +- Lookups are tenant-scoped, and the tool returns only an action id and, later, a `credential_ref`. + +Two parts of acceptance scenario D still fail: +- "duplicate requests are rejected" is not implemented; +- "foreign-target requests are rejected" does not hold once a policy is revoked or narrowed. + +## Blocking + +**B1. A revoked or narrowed policy does not stop open cards. They can still be answered for 7 days.** +- Evidence: + - The policy is checked only when the card is created (`internal/tools/credential_request_tool.go:230`). + - `coreRespond` (`internal/http/workspace_setup_respond_core.go:137-185`) checks the human gate, tenant, expiry and payload, then runs the applier. It never re-reads the agent's policy. + - The tool sets no `ExpiresAt`, so cards get `SetupActionDefaultTTL = 7 * 24h` (`internal/store/workspace_setup_store.go:79`). + - The behaviour is intentional and documented (`cmd/agent_credential_policy_cmd.go:113`, OpenAPI DELETE description: "open cards stay answerable by a person"). +- Failure scenario: + 1. A prompt-injected support agent has `channel_instance/telegram [update]`. It opens 20 update cards naming the production Telegram instance. + 2. The admin notices and runs `credential-policy clear`. + 3. Three days later another admin answers one of those cards. The applier rotates the production bot token. + 4. The same happens after an admin narrows the allowlist: cards for removed targets still apply. + + Revocation, which is the containment control, therefore does nothing to requests already issued. +- Fix (either option works; do both if possible): + - Mark cards made by the tool, for example `RequestedBy = "credential_request:"`, which the store already has. In `coreRespond`, before `ClaimAction`, re-load the agent and re-run `resolveTarget` and `policy.Allows` on the stored payload. Refuse with 409 and cancel the card when it no longer matches. + - When a policy is cleared or set, cancel open `pending` cards of that agent whose target is no longer allowed. + - Also give tool cards a short TTL, such as 1 to 24 hours. + - Add a test: create a card, revoke the policy, answer as owner, and expect a refusal with no provider row written. + +**B2. Duplicate requests are not rejected, and nothing limits how many cards an agent can create.** +- Evidence: + - The only de-duplication is the `(tenant_id, session_key, step_id)` partial unique index (`internal/store/pg/workspace_setup.go:140-146` and the sqlite mirror). It applies only when step_id is non-empty. + - `step_id` is optional and chosen by the agent (`credential_request_tool.go:240`). + - There is no per-agent, per-session or per-tenant limit on open cards, and no rate limit (the store has no limit check at all). +- Failure scenario: an agent calls `credential_request` in a loop without `step_id` (or with a new one each time). Every call stores a new 7-day secret card for the same target. The chat fills with identical credential prompts, which invites the user to answer a stale one, and the action table grows without bound. Acceptance D explicitly requires duplicates to be rejected. +- Fix: + - Derive a server-side dedup key from the target, for example `credreq::::`, and ignore or namespace the agent's `step_id`. An open card for the same target is then returned instead of a new one; `ErrSetupActionOpen` already supports this. + - Cap open tool cards per agent and session (for example 3) and per tenant, and record `credential_request.denied` with reason `rate_limited`. + - Add tests for both. + +## Should-fix + +**S1. A non-setup agent's `step_id` can move steps of the tenant's workspace setup plan.** +- Evidence: `applySecret` and `applyOAuth` run `stepProgressAfter` whenever `a.StepID != ""` (`internal/http/workspace_setup_secret_appliers.go:98-99`, `workspace_setup_respond_appliers.go:184-185`). `mutateSetupPlan` loads the tenant-wide plan (`workspace_setup_respond_appliers.go:193-206`). The step id comes straight from the agent (`credential_request_tool.go:240`). +- Scenario: a non-setup agent passes `step_id` equal to a plan step such as `provider`. When the person answers, the onboarding plan step moves to `running` or `done`. That is a capability only setup operators should have. +- Fix: never forward an agent-chosen `step_id` from this tool. Use the server-derived key from B2, with a prefix that cannot match a plan step. + +**S2. The HTTP policy routes take their role from API-key scopes, not from tenant membership.** +- Evidence: + - `adminMiddleware` is `requireAuth(RoleAdmin)` (`internal/http/agents.go:198-199`, used at `agents_credential_request_policy.go:72-74`). Its role comes from the key's scopes (`internal/http/auth.go:247,300,674`). + - `apiKeyTenantMembershipOK` only checks that the key owner has some membership, at any role (`auth.go:413-437`). + - The respond path additionally calls `requireTenantAdmin`, which checks for an owner or admin membership (`workspace_setup_handler.go:156`, `tenant_auth_helpers.go:22-33`). + - The implementer's report says the HTTP routes use `requireTenantAdmin`. The code does not. +- Scenario: a user who was an admin and was demoted to member keeps an `operator.admin` key. Through HTTP they can still widen an agent's credential allowlist, but they can no longer answer cards. +- Fix: call `requireTenantAdmin(w, r, h.tenants)` (or `isTenantAdminCaller`) in `allowCredentialPolicyWrite` and in the GET handler. Check the WS `client.Role()` path the same way, and add a test for a key with admin scope held by a member. + +**S3. An `mcp_server` grant covers every MCP server in the tenant.** +- Evidence: + - The policy has no identifier for `mcp_server` (`internal/store/agent_credential_request_policy.go:107,145`). + - The tool returns the target with an empty identifier for both the secret card and the OAuth card when `provider=="mcp"` (`credential_request_tool.go:270-272,293-294`), so `Allows` matches any `id` or `server_id`. +- Scenario: an agent granted MCP rotation for one server asks the user to re-key, or re-authorize, the tenant's other MCP servers, such as a GitHub server holding an admin token. +- Fix: require a `server_id` (or server name) in the `mcp_server` policy entry and match it against the card's `id` or `server_id`. Alternatively, document loudly that this grant covers all servers. + +**S4. The claim "the agent can never read the key" is weaker for `cli_agent_credential`.** +- Evidence: the applied secret is injected into the agent's own SecureCLI environment, and the agent chooses the CLI arguments. Output is protected only by exact-value scrubbing (`internal/tools/credentialed_exec.go:663-665`). +- Scenario: a granted `gh` credential, followed by `gh auth token` or `gh api` echoing the token in another encoding, can expose it beyond exact-match scrubbing. I did not prove a specific bypass; this depends on SecureCLI deny rules. +- Fix: state this residual risk in `docs/workspace-setup-onboarding.md#agent-credential-requests`. Consider letting the policy grant `cli_agent_credential` only for presets that have token-printing subcommands denied. + +**S5. The tests do not cover the security-relevant paths end to end.** Specifically: +- No test goes tool, then store, then respond with a non-setup agent. `credential_request_respond_human_gate_sqlite_test.go:1618` builds the card by hand with the Super Agent and the bootstrap key. +- No test covers revocation while a card is pending (B1), duplicates or spam (B2), or step_id collision with a plan step (S1). +- There is no PostgreSQL test for the jsonb SQL in `internal/store/pg/agents_credential_request_policy.go`; only sqlite has one. +- No test shows that `agents.update` (WS) or `PUT /v1/agents/{id}` with `other_config.credential_request` neither grants nor drops the policy. The store logic covers this, but the handler level does not. +- No test covers the self-evolution write in `internal/agent/evolution_guardrails.go:189`, which replaces `other_config`. + +**S6. Docs and OpenAPI do not match the behaviour.** +- OpenAPI and docs say "tenant owner or admin", but the HTTP check is scope-based (S2). +- The 403 `setup_human_actor_required` uses the generic `Forbidden` response, so the `error.details` shape is not described. +- `docs/workspace-setup-onboarding.md` and the permission matrix do not say that open cards survive revocation, that cards last 7 days, or that an `mcp_server` grant covers all servers. +- The implementer's report contradicts itself (its "Status: not compiled" section versus its later Verification). Fix it before this goes to the PR description. + +## Nit +- **N1.** WS writes invalidate the loop only on the local node (`internal/gateway/methods/agents_credential_policy.go:170`); HTTP broadcasts on the bus. This is safe, because the tool re-reads the policy, but on other nodes the tool stays visible until their cache expires. The same is already true of WS `agents.update`. Use the bus broadcast. +- **N2.** Pinned-version runs build the loop from snapshot `other_config` (`internal/agent/resolver_agent_version.go:214`), so tool visibility follows the snapshot, not the live policy. The execution check is live, so this is safe, but it is confusing. Compute `credentialRequest` from the live agent. +- **N3.** The card title and body are written by the agent (120 and 1000 runes, checked for secret-shaped text) and shown above a trusted secure form. A server-made header such as "Agent X requests: provider openai (create)" would make phishing-style wording obvious. +- **N4.** A generic update whose `other_config` is not an object (for example `[]`) silently drops the policy (`agent_credential_request_policy.go`, `TakeOtherConfigWrites`). This fails closed, but reject a non-object `other_config` at the handlers. + +## Checks that passed (verified) +- **Agent or non-admin changing its own policy.** + - HTTP `PUT` filters keys through `agentAllowedFields` (`internal/http/validate.go:43`), and WS `agents.update` uses a typed struct. Neither can send the virtual key. + - An `other_config` replacement is stripped of the policy and keeps the stored one (`TakeOtherConfigWrites`). This covers evolution and version rollback. + - `Create` strips the policy (`pg/agents.go:108`). + - Writes require a person through the `RequireHumanActor` agent-held-key lineage scan. + - The setup-policy CLI classes `set` and `clear` as Blocked. +- **Allowlist at card creation.** Checked after `uiaction.ValidatePayload`. The payload is re-marshalled from the typed struct, so the stored and applied payloads match. An update card's stored type must match the claimed type, and a CLI card must name the calling agent. +- **Cross-tenant isolation.** The tool compares `ag.TenantID` with the context tenant, the policy handlers return 404 / NOT_FOUND, the store writes add `AND tenant_id`, and the appliers check `p.TenantID != actor.TenantID`. +- **What the model sees.** The tool result is `ForLLM` with only the action id. The applier results are `SecretResult` / `OAuthResult`, which hold a `credential_ref` and no value. +- **Audit records.** Activity rows and span metadata hold only ids, kind, target type, identifier, mode, reason, session, run and trace. HTTP audit details hold `agent_key`, `enabled` and the target count. +- **Expired cards.** Refused (`workspace_setup_respond_core.go:157`). +- **Forbidden items untouched.** `setup_operator` is not widened, and the Origin, CSRF and human-actor guards are unchanged. + +## Recommended order +B1, B2 and S1 (one change, with server-derived keys and a check at respond), then S2, S3, the S5 tests and the S6 docs. + +## Unresolved questions +1. Is it intended that only tenant owners and admins can answer these cards? `respond` requires `requireTenantAdmin`, so a member chatting with the agent sees a card they cannot submit. +2. Can a tenant member mint an API key with admin scope? This sets how severe S2 is. + +Status: DONE_WITH_CONCERNS +Summary: The policy management surfaces are sound (admin plus a person, tenant-scoped, the generic update path cannot grant). Two acceptance-D properties fail: open cards stay answerable for 7 days after the policy is revoked or narrowed, and duplicate or spam requests are not rejected or rate-limited. +Concerns/Blockers: B1 (no policy re-check at respond), B2 (no server-side dedup or cap). Should-fix: plan step_id hijack (S1), HTTP role taken from key scopes (S2), MCP grant covers all servers (S3), missing end-to-end, revoke and PostgreSQL tests (S5). diff --git a/plans/reports/code-reviewer-261011-2145-extension-rereview.md b/plans/reports/code-reviewer-261011-2145-extension-rereview.md new file mode 100644 index 0000000..8827b9d --- /dev/null +++ b/plans/reports/code-reviewer-261011-2145-extension-rereview.md @@ -0,0 +1,79 @@ +# Re-review: Dewee Chrome MV3 extension (after review fixes) + +Scope: every finding in code-reviewer-261011-1910-extension-review.md, the claimed fixes, and regressions in src/**, wxt.config.ts, scripts/zip-store.mjs, .github/workflows/*, tests/unit + tests/integration. +Method: I read the code. Contract claims were checked against the Dewee Go source (`dewee/internal/http/sessions.go`). +Gates: NOT run. The Bash tool fails before it executes anything ("unexpected EOF"), so lint, typecheck and vitest are unverified. + +## Old findings + +| ID | Status | Evidence | +|---|---|---| +| B1 | Closed (no test) | ChatShell.tsx:54-55 memoizes on `profileId/kind/origin`. use-chat.ts:121-125 returns early when the session key is adopted back. Gap: no test mounts ChatShell or round-trips `onSessionKey` through settings. The tests stub `onSessionKey` with `vi.fn()`, so the props-change path is never exercised (see NEW-1). | +| S1 | Partially closed | The proactive path is fixed: client-factory.ts:17-20 keeps status 0, and tests cover it. Still open: on a server 401, dewee-client.ts:170 calls `auth.renew()`. That returns `false` on *any* worker failure, including `{ok:false,status:0}` from a network error during refresh (client-factory.ts:22-24). Line 173 then throws the original 401, and App.tsx:36 clears the credential without a revoke. Scenario: clock skew, or a server-side early expiry, triggers a 401, and Wi-Fi drops during the refresh. The user is signed out locally and the refresh family is left alive. | +| S2 | Closed | token-manager.ts:35-36 re-reads the credential inside the single-flight task and returns it unchanged if the refresh token rotated. Test at token-manager.test.ts:79. Residual (inherent, not a regression): if the worker is suspended after the server rotated but before `writeCredential`, R1 is replayed. | +| S3 | **Open** (not in the fix list) | schemas.ts:64-66 still accepts only string `cursor`/`next_cursor`. Dewee's follow log emits integers (`sessions.go:220-222`, `Cursor int`, `NextCursor int`, `Total int`). If the `/v1/extension` adapter reuses it (no adapter exists yet: grep for `/v1/extension` in dewee/internal finds nothing), then the new length probe (history.ts:20) and every page fail Zod with `invalid_response`, and no conversation loads. The response also carries `total`, so the extra probe request is unnecessary. docs/api-contract.md:85 still shows strings. | +| S4 | Closed | history.ts:28 starts at `max(0, total-1000)`. The page cap is 7. Tests cover the long-log, shrink and no-length cases. | +| S5 | Closed | use-chat.ts:229-244 queues the continuation and drains it on idle and on `streamsEnded`. `agent_key` is passed through (line 237). Double-fire analysis: if the effect runs while `streamRef` is still set, it re-queues. Once `runStream` sets `streamRef` synchronously and `pending` is null, a second run is a no-op. The epoch reset clears `pending` (line 130). Test at use-chat.test.tsx:160. | +| S6 | Partially closed | The happy path works and has a test. See NEW-1 and NEW-2 for the gaps. | +| S7 | Closed | MessageList.tsx:102-105 has a retry for `loadError`, wired to `chat.reload`. | +| S8 | Closed | The release `build` job has `persist-credentials: false` and a read-only token (release.yml:7-18). `contents: write` is only in the `publish` job, which neither checks out nor installs. The store build drops `key` (wxt.config.ts:8,19; zip-store.mjs:4) into `.output-store`, renamed `-store.zip`. | +| S9 | Closed | origin.ts:9 accepts only `localhost`/`127.0.0.1`, which matches wxt.config.ts:25. | +| N1 | Closed | SessionList.tsx:63 gates the button with `canLoadMore`. | +| N2 | Closed | ChatShell.tsx:148. | +| N3 | Closed | chat-state.ts:198-205 `idle` clears `streaming`. | +| N4 | Closed | use-chat.ts:156 checks `streamRef.current`. | +| N5 | Closed | Documented as ADR-001 in docs/development.md:68. | +| N6 | Partially closed | CredentialsForm.tsx:27 prefers typed ids but still falls back to agent-supplied `target.name`. | +| N7 | Closed | i18n.ts:86-89 (`conflict`, `profile_not_found`, `background_unavailable`, plus vi copy). | +| N8 | Closed | connect.ts:46-52 clears the credential when `saveProfile` fails. | +| N9 | Partially closed | settings.ts:63-77 serializes writes **per JS context** only. The panel and the options page each have their own `writes` chain, so a theme change in options racing a session write in the panel can still lose one update. Low impact. | +| Retry-after | Closed | use-chat.ts:210 → chat-state.ts:191 → MessageList.tsx:21 `errorText(..., retryAfterSeconds)`. | +| App onAuthError | Closed | App.tsx:32 is a plain function. Each consumer either reads it through a ref (use-chat) or captures a closure for the same profile (use-directory, ChatShell `key={profile.id}`), so a stale closure is harmless. | + +## New findings + +### Blocking +None. + +### Should-fix + +**NEW-1. Stop before `chat.started` in a new chat resets the conversation and leaks a stale `runId`** (use-chat.ts:176-188) +- Line 178 calls `onSessionKey`, and that starts an async `update()`. Line 183 then `await`s the network `abortOnServer` *before* line 188 dispatches `chat.started`. +- The storage write usually finishes first. ChatShell re-renders with `ctx.sessionKey = S`, and the layout effect overwrites `stateRef.current` with reducer state that has no `sessionKey` yet, undoing the mutation at line 177. +- The scope effect at line 125 then sees `S !== undefined` and runs a full reset: epoch bump, stream aborted, `loadSession`. The "Stopped" bubble and the local user message are replaced by history. +- After the await, line 188 dispatches `chat.started` into the *new* conversation with no `isCurrent` check, so `runId = r_old` is stored. If the status poll says the run is not running, nothing clears it: `history` and `send` keep `runId`. +- Next turn: the user presses Stop before the new `chat.started`. `stop()` (line 248) sees a `runId`, so it calls `abort(S, r_old)` and aborts the fetch locally. The new run keeps going on the server, which is exactly the orphan S6 was meant to prevent. +- The integration test misses this because `onSessionKey` is a spy and never changes props. +- Fix: dispatch the event before handling the stop request, re-check `isCurrent(epoch)` after the await, and clear `runId` in the `send` reducer case. Add a test whose `onSessionKey` re-renders with the new session key. + +**NEW-2. Stop waits for a `run_id` that may never come, so the server run is never aborted** (use-chat.ts:248) +- The deferral is keyed on `!runId`, but `runId` is optional (chat-events.ts:60,140). +- If the Gateway sends `chat.started` without `run_id`, the event has already been consumed. `stop()` sets `stopRequestedRef` and waits for a second `chat.started` that never arrives, and after 5 s it aborts only locally. +- The same applies to an existing session (where `sessionKey` is already known) while `chat.started` is late: the fallback at lines 253-258 never calls `abortOnServer(sessionKey)`. +- Fix: track "started seen" in a ref, defer only until then, and in the timeout call `abortOnServer` whenever a session key exists. + +### Nit +- **NEW-3** use-chat.ts:241-244, 261-262. A card answered mid-stream and then Stop: the stopped stream ends (`chat.stopped` → idle, `streamsEnded++`), and the queued continuation starts a new run immediately after the user pressed Stop. Decide the intended behavior. If Stop should hold it, clear `pendingContinuationRef` in `stop()` and offer a "Continue" action. +- **NEW-4** use-chat.ts:209. A continuation that fails without a reconcile (for example a 429 right after the queue drains) has `retryText` undefined, so there is no retry button. The setup action has already been consumed, and the agent never resumes. Offer a "Continue" retry that carries `setup_action_id`. +- **NEW-5** ChatShell.tsx:122. The lost-response replay builds the continuation with the *current* `agentKey`, not the card's agent. Low risk while cards share the session agent. + +## Hard rules (verified) +- **Secrets:** No `console`, `localStorage`, `storage.sync`, `eval`, `new Function`, `executeScript` or `tabs.` in src. Secret values stay in CredentialsForm local state, are cleared before the await (CredentialsForm.tsx:54), and go only to `respond`. The respond retry path never re-sends the body. Credentials live in `storage.session`, or `storage.local` only with the opt-in. +- **No master token:** the gateway flow verifies the key with `/me` and requires `auth.mode === 'api_key'` (connect.ts:44). +- **No agent HTML:** the only `dangerouslySetInnerHTML` is MessageList.tsx:13, which goes through `renderMarkdown` (DOMPurify). +- **Extension policy:** CSP is `script-src 'self'; object-src 'self'; base-uri 'none'; frame-ancestors 'none'`. There is no content script, and no `tabs`/`scripting`/`activeTab`. Host access is requested per origin through `optional_host_permissions`; `host_permissions` exist only in the e2e build. +- **Release token:** passes, as described under S8. + +## Recommended actions +1. Fix NEW-1 and NEW-2, and add a test that re-renders with the adopted session key during a Stop. +2. S1: make `renew()` throw a status-0 error on transport failure, or stop clearing the credential in App and let the worker own clearing. +3. S3: accept `int | string` cursors, use `total` instead of the probe, and pin the JSON type in api-contract.md together with the Dewee adapter issue. +4. Run `pnpm lint && pnpm typecheck && pnpm test && pnpm build`. This review could not. + +## Unresolved questions +1. Will the `/v1/extension` history adapter emit integer cursors and `total`? This decides S3. +2. Should a continuation queued before Stop still run? (NEW-3) + +Status: DONE_WITH_CONCERNS +Summary: 14 of 20 prior items are closed. S1, S6, N6 and N9 are partially closed and S3 remains open. Two new Stop-path races in use-chat.ts can still orphan a server run (NEW-1 also leaks a stale runId). No blocking issues and no hard-rule violations. +Concerns/Blockers: The shell does not work, so no gates were run. S3 depends on the unbuilt Dewee adapter. diff --git a/plans/reports/code-reviewer-261011-2150-dewee-991-review.md b/plans/reports/code-reviewer-261011-2150-dewee-991-review.md new file mode 100644 index 0000000..0bc1e80 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2150-dewee-991-review.md @@ -0,0 +1,49 @@ +# Review: dewee#991 workspace chat runtime (459633e5 on 57026762) + +Scope: `internal/http/provisioning_chat_runner.go`, `provisioning_chat_stream.go`, their tests; read-only cross-check of `provisioning_chat.go`, `provisioning_chat_abort.go`, `agent/router.go`, `gateway/methods/chat.go`, `http/chat_completions.go`. The shell was broken in this session (bash EOF error), so I could not run `go test`/`-race`/`vet`. All findings come from reading the code. + +Verdict: the three fixes are correct and mirror `chat.send`. No blocking defect in the diff itself. Abort is newly reachable, which exposes two semantic gaps (S1, S2), and the merge with #956 conflicts textually and breaks compilation if resolved naively (section C). + +## Verified OK +- Registration parity: `provisioning_chat_runner.go:48-56` = `gateway/methods/chat.go:444-460` (WithoutCancel, then InjectTeamDispatch, then WithCancel, then RegisterRun on the base ctx so TenantID is captured). `InjectCh` (:67) and `OnTraceCreated`→`SetRunTraceID` (:68-70) are wired. `defer UnregisterRun` and `defer cancel` cover the success, error and panic paths. `router.Get` failure returns before RegisterRun, so nothing leaks. A fence rejection in RegisterRun cancels and closes injectCh, the same as the other callers. +- Boundedness: the run ends on loop completion, router abort or process exit. It is the same as `chat.send`: no wall-clock cap (already noted by the implementer). No goroutine leak: the handler goroutine stays pinned until the run returns, then exits. No http.TimeoutHandler or request-ctx timeout wraps this route (grep: none in `internal/http`). +- Write after return: OnEvent runs synchronously inside `RunWorkspaceChat`, and parallel tool goroutines are joined (`pipeline/tool_stage.go:275` WaitGroup) before return, so nothing writes after the handler returns. A write in the TOCTOU window between the `done` check and `Fprintf` (`stream.go:78-88`) goes to a still-live handler's ResponseWriter and just returns an error. That is safe. +- Setup-turn side effects survive disconnect: the lease uses WithoutCancel (`provisioning_chat_setup_turn_lease.go:59`) and `markSetupHandoff` uses WithoutCancel (`provisioning_chat_setup_turn.go:142`). +- Bug 1: the loop emits `map[string]string` (`agent/loop_pipeline_callbacks.go:430,443,573,588,600`, `loop_pipeline_adapter.go:318,329`), and `chunkContent` handles both shapes. +- Tests would fail without the fix: the mapper cases for `map[string]string` return ""; `RegistersRunForStatusAndAbort` fails because `IsSessionBusy` returns false; `SurvivesRequestCancel` fails because cancel propagates synchronously, so ctxErr=Canceled whichever select branch wins. Channel handoffs make them race-free. + +## Blocking +None in the #991 diff. + +## Should-fix +S1. The buffered (non-stream) path maps a user abort to HTTP 500 plus a retry hint. `provisioning_chat.go:286-300`. + Scenario: the CCP sends `stream:false`, the user hits Stop, and `/workspace-chat/abort` cancels the run. `loop.Run` returns a ctx error, which `ClassifyWorkspaceChatRecovery` maps to `runtime_request_failed`/`retry`, so the caller gets a 500 "request failed, retry" and may auto-resend the turn the user just stopped. Before #991 this path was unreachable, because runs were not registered. + Fix: when `errors.Is(err, context.Canceled)` (or the runner surfaces "aborted"), return 200 with `{stopped:true}` or 409 with a dedicated code. Skip `setupTurn.runFailed` on an abort, or pass the abort to it explicitly. +S2. On the stream path, a setup turn that is aborted can emit two terminal frames. `provisioning_chat_stream.go:121-128,155-166`. + Scenario: the run is aborted after `OnInputDelivered`. `chat.stopped` is sent immediately (it is not held), then `runFailed(true)` sets `delivered`, so a second `chat.error setup_result_delivered` follows. Clients that end the stream on the first terminal frame drop the second, and others render an error after "stopped". Fix: when `terminalSent` came from `chat.stopped`, do not send the delivered error, or hold `chat.stopped` the same way `chat.error` is held. +S3. There is no busy guard before starting a run on an existing session. `provisioning_chat.go:218-232` (compare #956 `extension_api_chat.go:102`). + Scenario: the client disconnects (the run continues, by design), reconnects and resends the turn. Two concurrent runs then write the same session history. `RegisterRun` overwrites `sessionRuns[key]` (`router.go:344`). When the first run finishes, `UnregisterRun` deletes the index unconditionally (`router.go:365`), so status reports "idle" while run 2 is still streaming. Fix: return 409 when `IsSessionBusy(sessionKey)`, matching the extension route. Optionally make `UnregisterRun` delete only when the index still points to its own runID (CompareAndDelete). +S4. (Pre-existing, now reaches workspace runs; file a separate `security` issue.) The router indexes runs by session key across all tenants. WS `chat.abort`/`chat.sessionStatus` let any tenant admin skip the session check (`gateway/methods/access.go:30`, `chat.go:93-101,746-766`, `AbortRun(runID,"")`). A tenant-A admin who knows a tenant-B workspace session key or runID can read B's run state or abort B's run. Exploiting it needs the key: `agent:{key}:app:direct:{user}-{8 hex}` has a 32-bit random suffix, and run IDs are UUIDv4. The provisioning abort route itself is tenant-scoped (`provisioning_chat_abort.go:121-130`). Fix: store TenantID in `ActiveRun` (already captured) and require it to match in `AbortRun`/`SessionRunID`/abort-by-runID. +S5. (Pre-existing, separate issue.) `sseEventWriter` is not safe for concurrent use, yet `tool.call` is emitted from parallel tool goroutines (`agent/loop_pipeline_tool_callbacks.go:123` via `pipeline/tool_stage.go:278`). This contradicts the OnEvent invariant at `loop_types.go:804-813`. Scenario: two parallel-eligible tools produce interleaved `Fprintf`/`Flush` calls on one ResponseWriter, which is a data race that can corrupt SSE frames. #991's `done` read is race-free, but it does not address this. Fix: add a mutex in `send`. +S6. The tests do not cover the handler wiring. No test drives `handleProvisionWorkspaceChatStream` with a cancelled `r.Context()` to prove (a) the run completes and (b) no frames follow. A regression to `newSSEEventWriter(w, nil)` or to a non-detached runner in the handler would pass the suite. Add one httptest case using `req.WithContext(cancelCtx)` and a blocking fake runner. + +## Nit +N1. `chunkContent` (`stream.go:278`) duplicates `channels.extractPayloadString` (`channels/events.go:687`). This is acceptable across packages; mention it in a comment or move it to a shared helper later. +N2. Defer order: `drainTeamDispatch` runs after `UnregisterRun` (`runner.go:50,56`), so status shows idle while post-turn team dispatch is still running. `chat.go:458-460` does the reverse, and `chat_completions.go` matches #991. Pick one order. +N3. The comment at `runner.go:46-47` says the run is stopped "only by the router abort". A tenant-transfer fence in `RegisterRun` also cancels it. + +## C. Compatibility with #956 (5e3e03ae, a078e857) when both merge into dev +Textual conflicts (git will stop): +1. `provisioning_chat_stream.go`. #956 adds `RunContext/TraceName/Surface` to the params struct (956 :61-68), right above the `sseEventWriter` struct and comment that #991 rewrote (991 :62-75). #956 also moves the handler body into `streamWorkspaceChat` (956 :110-185), while #991 edits that body (`:109`) and the handler doc comment (`:94-100`). The mapper's chunk/thinking lines (991 :197/:199 vs 956 :210/:212) should merge cleanly. +2. `provisioning_chat_runner.go`. #956 inserts the `traceName` block at 956 :44-49 and changes `TraceName: traceName` (:66). #991 inserts the WithoutCancel/RegisterRun block at the same spot (:44-56) and adds `InjectCh`/`OnTraceCreated` around `TraceName` (:67-77). +Compile failure if the merge keeps #956's body: `streamWorkspaceChat` calls `newSSEEventWriter(w)` (956 :118), but #991's signature is `newSSEEventWriter(w, done <-chan struct{})`, so the build fails with "not enough arguments". +Required resolution on the #956 side: +- In `streamWorkspaceChat`, write `sse := newSSEEventWriter(w, r.Context().Done())`. Use `r.Context().Done()`, not `params.RunContext.Done()`: the extension's RunContext is already detached, so its Done() is nil and frames would never stop. Keep #991's doc comment ("run detached; disconnect only stops writes") on `streamWorkspaceChat`, not on the thin wrapper. +- Runner: keep #956's `traceName` default and #991's `runCtxBase := context.WithoutCancel(ctx)` → `InjectTeamDispatch(runCtxBase, …)` → `WithCancel` → `RegisterRun` → `InjectCh`/`OnTraceCreated`/`TraceName: traceName`. +- `extension_api_chat.go:120`: `context.WithoutCancel(ctx)` becomes redundant because the runner detaches. It is harmless, so drop it or keep it. Update the comment at :116-119: no "runtime run timeout" exists, and abort now works through router registration. Keep `WithoutCancel` at :160, because it covers the lease, not the run. +- Behaviour change that #956 gains for free: its `IsSessionBusy` 409 guard (:102), status route (`extension_api_read.go:375-376`) and abort route (:195-197) only start working for real after #991. Before that, workspace runs were never registered. #956's SQLite tests use a fake runtime (`extension_api_sqlite_test.go:75-101`), so they will not catch a wiring regression. Add one test against a real `agent.Router` after the merge. +- S1 applies to #956 only if it adds a non-stream route (it currently has none). S4 also covers extension sessions. + +Status: DONE_WITH_CONCERNS +Summary: The #991 fix is correct and mirrors chat.send (registration, detach and delta mapping), and the tests are meaningful. Newly reachable abort exposes a buffered-path 500/retry and a double terminal frame on aborted setup turns, and there is no same-session busy guard. +Concerns/Blockers: I could not run go test/-race (shell broken). The merge with #956 conflicts in the stream and runner files and fails to compile until streamWorkspaceChat passes r.Context().Done(). The cross-tenant admin abort/status issue (S4) is pre-existing and needs its own security issue. diff --git a/plans/reports/code-reviewer-261011-2230-dewee-992-993-review.md b/plans/reports/code-reviewer-261011-2230-dewee-992-993-review.md new file mode 100644 index 0000000..398cb5a --- /dev/null +++ b/plans/reports/code-reviewer-261011-2230-dewee-992-993-review.md @@ -0,0 +1,100 @@ +# Code review: dewee#992 (6690a9b0) and dewee#993 (789895f1) + +Isolated read-only review. Diffs: `git diff fcab6128 6690a9b0` (worktree dewee-992-own-sessions) and +`git diff 57026762 789895f1` (worktree dewee-993-respond-body-cap). + +Verification run: +- `go test ./internal/http -run 'ProvisioningHandlerSessions|ProvisioningHandlerReportsRecentlyActive|ProvisioningWorkspaceSessions'`: ok +- `go test -tags sqlite ./internal/http -run ProvisioningWorkspaceSessionsOwnOnly -v`: PASS +- vitest `runtime-sessions-client.test.ts` + `extension-api-routes.test.ts`: 40/40 pass +- vitest `setup/setup-routes.test.ts` (993): 66/66 pass +- `go clean -testcache` run afterwards. + +## Commit 6690a9b0: own_only / agent_key session filter (dewee#992) + +### Blocking +None. + +### Should-fix +1. **Valid agent keys are rejected by the slug check.** `internal/http/provisioning_sessions.go:116` returns 400 for an + `agent_key` that does not match `isValidSlug` (`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`), and the BFF route returns an empty + list for the same pattern (`apps/dewee-app/src/pages/api/extension/v1/workspaces/[workspaceId]/sessions.ts:20,31`). + Agents created via the provisioning path use `config.NormalizeAgentID` (`internal/http/provisioning_agents_crud.go:368`), + whose pattern is `^[a-z0-9][a-z0-9_-]{0,63}$` (`internal/config/normalize.go:11`). That pattern allows `_` and a + trailing `-`. So a CCP agent named "sales_bot" gets key `sales_bot`. Its chats can no longer be filtered by agent: the + extension gets `{sessions: [], total: 0}`. Before this change, the BFF compared keys exactly and only enforced + length ≤ 128. This is a functional regression, not a security problem. + Fix: accept the `NormalizeAgentID` shape and escape LIKE metacharacters in the store. Use `ESCAPE '\'` and escape + `\`, `%` and `_` in `buildSessionFilter` for PG (`internal/store/pg/sessions_list.go`) and SQLite + (`internal/store/sqlitestore/sessions_list.go`). Alternatively, validate against `validIDRe` and escape `_`. Mirror + the same pattern in the BFF and update the docs. +2. **The docs claim does not match reality.** `docs/ccp-session-detail-inspection.md:17` says `agent_key` "must be a + slug". `docs/extension-oauth-and-bff.md` says a non-slug key "lists nothing". Both describe the regression above as + intended behaviour. Update them with the fix. + +### Nit +3. **List rule differs from the detail rule.** The SQL own filter is `user_id = actor AND session_key LIKE 'agent:%:app:%'` + (`provisioning_sessions.go:163-165` with the store `Channel` LIKE). `isOwnAppChatSession` + (`provisioning_session_detail.go:171`) instead uses the `channel` column. Both require `user_id = actor`, so no + cross-user or cross-tenant row can leak. A session can still be listed and then refused on detail, or the reverse, + when the key and the column disagree (for example a caller-supplied `SessionKey` in `provisioning_chat_runner.go:33`). +4. **`total` can exceed what the BFF returns.** The BFF still post-filters with `mayBeOwnAppChatSessionKey` + (`agent:[^:]+:app:direct:-…`), which is stricter than the SQL LIKE (that also matches `app:group:…` or + custom keys). The route returns the gateway's `total` (`sessions.ts:43`). With such rows, a page can be short while + `total` still counts them. This only happens with unusual keys. Note it in a comment or count only after the + guard. +5. **Unconfirmed agent filter.** `agent_key` without `own_only` gets no confirmation echo (`own_only_enforced` only). + No caller does this today. If a future caller relies on `agent_key` alone, an older gateway would silently ignore it. +6. **Unbounded offset.** BFF `offset` max is now 1,000,000 (`sessions.ts`), and the gateway has no upper bound. It is + indexed (`idx_sessions_tenant_user`), so the cost is acceptable. A tighter bound would still be cheap insurance. + +### Checks that passed +- **Authorization:** `own_only` binds `filter.UserID = actorUserID`. The actor comes from the HMAC-signed provisioning + body after the membership check, and the tenant comes from `binding.TenantID`. The BFF always sends `ownOnly: true` + with `actorUserId: userId` from the bearer token. Client query input cannot widen the scope. The sqlite test covers + a same user_id in another tenant, other users, and other channels. +- **SQL:** every value is parameterised (`$n` / `?`). `agent_key` reaches only a bound LIKE argument, and the slug check + blocks `%`/`_`. Postgres and SQLite paths are symmetric. +- **Pagination:** `total` and `active_count` come from the same filter (`page := filter`, `active := filter`). +- **Backward compatibility:** with neither field set, the filter equals the old one (Channel and TenantID). + `own_only_enforced` is `omitempty` and absent, as the test asserts. Other `listSessions` callers do not send the new + fields (test "sends neither filter"). +- **Fail-closed:** `runtime-sessions-client.ts:170` throws 503 `runtime_session_scope_unavailable` unless + `own_only_enforced === true`, checked after the HTTP status. This is correct. +- **Tests would fail without the fix:** the Go tests reference `OwnOnly`/`OwnOnlyEnforced`, so they do not compile at the + base. The sqlite test totals would include other users. The BFF tests assert a single call with `ownOnly: true` and + the old-chat case. + +## Commit 789895f1: respond body cap at 160 KiB (dewee#993) + +### Blocking +None. + +### Should-fix +None. + +### Nit +7. **Escaped JSON can exceed the cap.** The cap is in serialized bytes, while the contract limits are in decoded bytes + (`workspace-setup-respond.ts:32-34`). A secret heavy in `"`, `\` or control characters expands under JSON escaping + by up to 2x (or 6x for `\uXXXX`). A contract-valid body can therefore exceed 160 KiB and get 413. Realistic secrets + (PEM, tokens) add only a few hundred bytes. Accept this, or say it in the comment. +8. **Comment refers to a route not on dev.** The comment at `action-respond.ts:15` says "the same cap as the extension + respond route". That route (`extension/.../actions/[actionId]/respond.ts:23`, 160 KiB) exists on feat/957 but not on + origin/dev. The reference is accurate only once feat/957 lands. + +### Checks that passed +- **Other routes:** the 64 KiB default is untouched and only `action-respond` passes a cap. The test "keeps the 64 KiB cap + on the other setup routes" covers `plan-approve`. +- **No echo:** the 413 body is `{error: "setup_request_invalid"}`, and the test asserts it contains no secret or filler. + `handleSetupRoute` logs only the error name. +- **CSRF and Origin:** `assertSameOriginMutation` runs first in `resolveSetupRoute` + (`workspace-setup-route-support.ts:62`), then the session and membership checks, and the body is read after them. + This is unchanged. +- **Downstream:** the gateway provisioning respond path reads with `maxProvisioningBodyBytes` (1 MiB, + `internal/http/provisioning.go:216`), so 160 KiB passes through. +- **Tests would fail without the fix:** the contract-maximum test asserts the body is over 64 KiB and expects 200, which + gives 413 at the base. + +Status: DONE_WITH_CONCERNS +Summary: No blocking or authorization issues. The own_only filter is bound to the signed actor and tenant, parameterised on both stores, and the BFF fails closed with 503. The 993 body cap is scoped to one route with the guards intact. +Concerns/Blockers: In 992, the slug-only agent_key check rejects valid provisioning-created agent keys that contain `_` or a trailing `-` (NormalizeAgentID). This is a filter regression, and the docs describe it as intended. diff --git a/plans/reports/code-reviewer-261011-2245-dewee-1000-review.md b/plans/reports/code-reviewer-261011-2245-dewee-1000-review.md new file mode 100644 index 0000000..be95cb1 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2245-dewee-1000-review.md @@ -0,0 +1,51 @@ +# Review: dewee a3d727b1 (fix/1000-cross-tenant-run-access), dewee#1000 + +Scope: `git diff origin/dev...HEAD` (7 files). This was a read-only review. A scratch export was used to revert the fix, and the scratch copy has been deleted. + +## Verdict +The fix is correct for chat.abort, chat.status, the cancel keyword, the HTTP concurrency cancel/status, and the workspace chat abort. +One cross-tenant abort path remains through the chat-concurrency steer intent, and one status oracle remains. + +## Blocking +1. **A cross-tenant abort still works through chat.send or HTTP using the steer intent.** See `internal/chatconcurrency/related.go:43` and `internal/chatconcurrency/coordinator.go:121`. + - `Prepare` calls the unscoped `router.IsSessionBusy(in.VisibleSessionKey)` (coordinator.go:121). This returns true when tenant B has a run on that key, even though tenant A's `ListActiveChatRunContexts` is empty. + - On `IntentSteer`, `selectActiveContext(empty, visible)` falls back to the caller-supplied key (session.go:39-41). `prepareRelatedRestart` then calls the unscoped `c.router.AbortRunsForSession(target.RunSessionKey)` (related.go:43), which aborts tenant B's run. + - Entry points: + - WS `chat.send` with `sessionKey` set to B's key. Ownership is not an obstacle: `m.sessions.Get(ctxA, keyB)` returns nil, so the call is treated as a new session (chat.go:258-263), and `agentId` can be the caller's own agent. + - HTTP `X-GoClaw-Session-Key` (chat_completions.go:181-187, and the same in responses.go) through `prepareExplicitChatConcurrency`. + - Exploitability: this is the same precondition as the issue (knowing B's session key). But it is broader, because **any authenticated member** can do it, not only an admin. The attacker controls the message, so the steer classification is easy to steer. It also needs a provider or usage caps so the classifier runs. The coordinator is non-nil in both PG and SQLite wiring (store/pg/factory.go:51, sqlitestore/factory.go:46), and `EffectiveEnabled` defaults to true. + - Fix: scope coordinator.go:121 with `IsSessionBusyInTenant(ctx, …)` and related.go:43 with `AbortRunsForSessionInTenant(ctx, …)`. Add a two-tenant test in chatconcurrency. If you would rather keep this PR narrow, open a linked `security` issue. Note that the comment in router_tenant_scope.go:14-17 currently claims that "cancel keywords" paths are covered. +2. **The same line, coordinator.go:121, is an existence oracle.** When B's key is busy, the cancel or status intent returns a "cancelled" or "idle status" reply and no run starts. When the key is free, a normal run is scheduled. That is a visible difference, so a cross-tenant run does not yet look like a missing run on this surface. Scoping line 121 fixes both problems. + +## Should-fix +3. **The mid-run injection at `internal/gateway/methods/chat.go:428-429` is still unscoped** (`IsSessionBusy` and `InjectMessage`). A member of tenant A could inject a user message into tenant B's run. This path is only reached when `m.concurrency == nil`, and production wiring always sets the coordinator, so the real exploitability is low. It is a one-line defence-in-depth change: gate it on `SessionRunIDInTenant` and add an `InjectMessageInTenant`. +4. **The `sessionRuns` index is shared across tenants.** `RegisterRun` overwrites `sessionRuns[key]` (router.go:351), and `UnregisterRun` deletes it by key (router.go:366). A tenant A run started on B's key (finding 1 path) therefore hides or clears B's index entry. B's own `IsSessionBusy`, `SessionRunID` and `InjectMessage` then misbehave, which is a cross-tenant disturbance or DoS. `SessionRunIDInTenant` works around this for reads, but the index itself should be keyed by tenant and session (for example `tenant:session`). This was already true before the change; it is the remaining root cause. +5. **The channel consumers in `cmd/gateway_consumer_normal.go` (lines 353, 502, 522, 548 and 946 via `abortSessionRunsForSender`) still use unscoped lookups.** + - The session key there comes from channel, peer and agent, so the caller cannot choose it. + - A collision needs the same peer, the same channel name and the same agent key in two tenants. That is effectively the same person on two bots, so exploitability is low. The result would be wrong-tenant steer injection, status or cancel. + - `ctx` already carries `msg.TenantID` (line 43), so switching to the `InTenant` variants is cheap. + - `cmd/gateway_events.go:345,354` (`SessionKeyForRun` from internal agent events) cannot be reached by a client. That is fine. +6. **Team and subagent paths.** No other router lookups exist: the grep over internal/ and cmd/ found only the sites listed above. Team and subagent runs go through the scheduler, using the consumer lookups in finding 5 and `Sched.CancelSession` (gateway_consumer_handlers.go:519), which is keyed by session only. No caller-chosen key reaches them. + +## Nit +7. Logging is asymmetric. `security.cross_tenant_run_abort_denied` (router_tenant_scope.go:48-51) fires only for run-ID aborts, not for session-key aborts or status probes. The logged fields are `run_id` and two tenant UUIDs. They contain no secrets and are safe to log. +8. `SessionRunIDInTenant` skips `State==2` in the fallback scan (line 97) but not on the fast path (line 88). This is harmless, but inconsistent. +9. Behaviour change: the owner role, which `IsMasterScope` treats as cross-tenant, no longer reaches runs in other tenants. Only the caller's ctx tenant counts. This fails closed and is consistent with the issue, but it is worth a line in the PR body. The `uuid.Nil` to Master mapping matches the tenant half of `store.IsMasterScope` (context.go:507-516). Non-master callers never match runs with a nil tenant (test at router_tenant_scope_test.go:62-72). WS clients always get a tenant (gateway/router.go:236-237 and 322-323). +10. The workspace chat runner calls `loop.Run` directly and never calls `RegisterRun` (provisioning_chat_runner.go:46). So the provisioning abort only reaches router-registered runs on the same app session key. This is pre-existing and outside this fix. `tenantCtx` always carries `binding.TenantID` (provisioning_chat_abort.go:96), so the changed interface is sound. + +## Checks +- Not-found equivalence: a cross-tenant run-ID abort returns `{RunID, NotFound}` before the session check, so there is no `Unauthorized` leak. The session paths return an empty list, the same as for a missing run. The timing difference is one extra `sync.Map` load plus a log line (microseconds, not observable over WS or HTTP). Error codes are identical. +- TOCTOU between the tenant check and `AbortRun`: run IDs are UUIDs and never reused, so this is not an issue. +- Same-tenant behaviour is unchanged. `TestChatAbortSameTenantStillAborts` and the same-tenant asserts in the agent test pass. Runs register with the caller's ctx tenant on every surface (chat.go:454, the HTTP handlers, and the scheduler's `pending.Ctx`). +- Tests would fail without the fix. I verified this by putting the origin/dev `chat.go` into a scratch copy of HEAD: all 4 `TestChatAbortCannotReachAnotherTenantsRun` subcases and `TestChatStatusHidesAnotherTenantsRun` fail. The agent and http tests depend on the new API, so they would not compile without it. The http test only checks that the tenant is passed through; there is no real-router two-tenant test for the provisioning abort. +- There is no test for the chat.send cancel or steer path, or for chatconcurrency (see finding 1). + +## Test runs (Windows) +- `go vet` on the 3 packages is clean. `internal/gateway/methods` passes. +- `internal/agent`: only `TestGetForRunEvictsLeastRecentlyUsedPinnedLoops` fails, and it fails identically on origin/dev. +- `internal/http`: 10 failures (symlink, media, SecureCLI, SSH CRLF, skill export). The same 10 fail on origin/dev, so they are the known Windows failures tracked in dewee#936. +- I ran `go clean -testcache` afterwards, and the worktree is unchanged. + +Status: DONE_WITH_CONCERNS +Summary: The fix correctly scopes chat.abort, chat.status, the cancel keyword, the HTTP cancel/status and the workspace abort to the caller's tenant, and its tests are real regressions. One blocking path remains: the chat-concurrency steer path (coordinator.go:121, related.go:43) still lets any member abort, or probe, another tenant's run by session key. +Concerns/Blockers: Scope coordinator.go:121 and related.go:43, and add a test, or explicitly move them to a linked security issue. The tenant-shared `sessionRuns` index (router.go:351, 366) is the remaining root cause. diff --git a/plans/reports/code-reviewer-261011-2315-dewee-1000-rereview.md b/plans/reports/code-reviewer-261011-2315-dewee-1000-rereview.md new file mode 100644 index 0000000..1cc0b93 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2315-dewee-1000-rereview.md @@ -0,0 +1,77 @@ +# Re-review: dewee fix/1000-cross-tenant-run-access (a3d727b1 + 3f9fa5dd), dewee#1000 + +This was a read-only review of `git diff origin/dev...HEAD` (11 files). Scratch copies were made with `git archive` and have been deleted. I ran `go clean -testcache`, and the worktree status is clean. + +## Verdict +All blocking findings from the previous review are fixed, and the fixes are covered by tests that fail without them. No path a client can reach still does an unscoped run lookup. There are no blocking findings, only a few nits. + +## Previous findings +- 1 and 2 (steer abort and existence oracle): fixed. + - `coordinator.go:121` now calls `IsSessionBusyInTenant(ctx, …)`, and `related.go:43` now calls `AbortRunsForSessionInTenant(ctx, …)`. + - `TestPrepareIgnoresAnotherTenantsBusySession` covers the steer, cancel and status intents. It also asserts that the decision equals the idle decision and that the classifier is never called, which closes the oracle. `TestRelatedRestartAbortsOnlyTheCallersTenant` covers the restart abort. +- 3 (mid-run injection): fixed. `chat.go:428-429` uses `IsSessionBusyInTenant` and `InjectMessageInTenant` with `primary.ctx`. +- 4 (shared `sessionRuns` index): out of scope, tracked as dewee#1008. `sessionRunInTenant` works around it correctly (router_tenant_scope.go:112-127). +- 5 (channel consumers): fixed at gateway_consumer_normal.go:353, 502, 522, 548 and 946. `ctx` carries `msg.TenantID`, or Master when that is nil (lines 42-46). +- 7 (log asymmetry): fixed. All hidden-run paths now log `security.cross_tenant_run_access_denied` through `logCrossTenantRunHidden`. +- 8 (fast path ignored `State==2`): fixed at router_tenant_scope.go:114, and covered by `TestSessionRunIDInTenantSkipsFinishedIndexedRun`. +- Test gap (no real-router provisioning test): fixed by `TestProvisioningHandlerAbortIgnoresAnotherTenantsRun`, which runs both the session and run-ID variants and then a positive abort in the caller's own tenant. + +## Verification +- Repo-wide grep for router lookups by run ID, session key or activity, run on both HEAD and the current origin/dev (c4c5c6c8). The only unscoped callers left are `cmd/gateway_events.go:345,354`. + - That code runs inside the bus subscriber for `EventAgent`. The only broadcaster of that event is the Loop `OnEvent` callback (gateway_managed.go:358), so a client cannot reach it. + - Run IDs are generated by the server everywhere a run registers: `uuid.NewString()` in chat.go:367, chat_completions.go:174, responses.go:110, wake.go and webhooks_llm.go:283, and an `inbound-…-uuid8` ID at gateway_consumer_normal.go:276. + - Newer commits on dev add no new callers. +- The ctx tenant is always set: + - WS: router.go:136. Every successful connect path sets `client.tenantID` (lines 237, 253, 323, 340, 359, 396 and 1007), and `resolveTenantHint` never returns Nil on success. + - HTTP: `enrichContext` maps Nil to Master (auth.go:571-575). + - Channel consumer: gateway_consumer_normal.go:42-46, which also flows through the scheduler's `pending.Ctx` (queue.go:163, 271). + - Provisioning: `tenantCtx` comes from `binding.TenantID` (provisioning_chat_abort.go:96). +- The Nil-to-Master mapping (`runTenantKey`) only matters for a caller whose ctx tenant is Nil or Master, and every non-master path above sets a real tenant. Owner and cross-tenant flags are never consulted, so neither can bypass the check (`TestTenantScopedLookupsTreatMissingTenantAsMaster`). +- Concurrency of the new scans: + - `sync.Map.Range` is safe to run concurrently with Store and Delete. + - The callback only reads fields that never change after `RegisterRun` (`RunID`, `SessionKey`, `TenantID`) and the atomic `State`. + - `AbortRunsForSessionInTenant` collects the runs first and then aborts them outside `Range`. The old code waited up to 3s inside `Range`. + - `InjectCh` is never closed for a run that is in the map (it is closed only at router.go:330, before the Store), so sending to it cannot panic. + - Checking the tenant and then calling `AbortRun` (TOCTOU) is safe, because run IDs are unique UUIDs. +- Same-tenant behaviour: `TestChatAbortSameTenantStillAborts`, the positive branch of the provisioning test and `TestRelatedRestartAbortsOnlyTheCallersTenant` (the caller's own run is still restarted) all pass. + - Owner role: chat.abort no longer reaches runs in other tenants. I found no UI that depends on that. The traces page's abort (traces-page.tsx:120) only shows traces from the caller's own tenant, because the tracing store filters by tenant and `WithCrossTenant` is only used by the skills store. This is not a regression. + +## Tests fail without the fix (scratch copy of HEAD) +- I reverted coordinator.go and related.go to a3d727b1. All 3 subtests of `TestPrepareIgnoresAnotherTenantsBusySession` failed (the result was cancel/status/steer handling instead of schedule). `TestRelatedRestartAbortsOnlyTheCallersTenant` failed with "tenant A's steer aborted tenant B's run". +- I reverted the fast-path State check and made `InjectMessageInTenant` delegate to the unscoped `InjectMessage`. `TestSessionRunIDInTenantSkipsFinishedIndexedRun` and `TestInjectMessageInTenantReachesOnlyTheCallersRun` both failed. +- In the previous review, the chat.abort and chat.status tests already failed against the origin/dev `chat.go`. + +## Test runs (Windows) +- `go vet` is clean on agent, chatconcurrency, gateway/methods, http and cmd. +- chatconcurrency and gateway/methods pass. +- internal/http has 10 failures, the same Windows set as the previous review (symlink, media, SecureCLI, SSH CRLF, skill export, tracked in dewee#936). +- cmd has 3 failures (`TestOnboardEnvFileSurvivesShellSourcing`, `TestShellQuoteEnvValueRoundTripsThroughBash`, `TestRGDGrantKeySourceErrors`). They fail identically on an origin/dev scratch copy. +- internal/agent: + - `TestGetForRunEvictsLeastRecentlyUsedPinnedLoops` fails on HEAD every time but passes on the current origin/dev. Dev fixed it after this branch's merge-base (58a73c0f) in d83e9519 (#949, coarse-clock eviction). The cause is that the branch is 11 commits behind dev, not this change. None of the files changed on dev overlap with this branch. + - `TestJevIntentProvider_FailsClosedOnVendorFailures` failed once in the full run under load and passed in isolation. It is a timing flake and unrelated. + +## Blocking +None. + +## Should-fix +None. + +## Nit +1. **Log volume and noise.** See router_tenant_scope.go:31-34. + - The WARN `security.cross_tenant_run_access_denied` fires on every lookup that hides a run. One `chat.status` probe can emit up to 3 WARN lines, plus more for each run session (chat.go:97-137). + - Any member can trigger these at will, so the log can be flooded, though only at the rate of their own requests. + - Benign collisions on channel keys (the same peer, channel name and agent key in two tenants) will also produce "security" warnings. + - Consider logging once per request, or at Info level for status and busy probes. +2. **Logging the session key is acceptable.** + - It is an identifier, not a secret, and the code already logs it elsewhere (for example the wake.go "wake request" log and related.go:45). + - On client paths the caller supplied the key, so the log discloses nothing new. + - Channel keys embed the platform peer or chat ID, which is pseudonymous PII. That matches the existing logging practice. + - No message content, tokens or credentials are logged. +3. **The call-site swaps in cmd/gateway_consumer_normal.go and the injection at chat.go:428 have no direct handler tests.** They are covered only by the router-level helper tests. The risk is low because the swaps are mechanical one-liners. +4. **Every chat.send that reaches `Prepare` with no active context now scans `activeRuns` in full.** This happens when the index misses, which is the common idle case (router_tenant_scope.go:56-72). It is O(active runs), which is fine at the current scale. The scan is needed for correctness until dewee#1008 keys the index by tenant. +5. **The commit and PR text do not mention the owner-role behaviour change.** A system owner can no longer abort or inspect runs in other tenants through chat.abort or chat.status. Add one line to the PR body. +6. **Rebase onto the current origin/dev before merge.** The branch is 11 commits behind, and the stale merge-base is what makes `TestGetForRunEvicts…` fail locally. + +Status: DONE +Summary: Both commits fix every finding from the previous review, including the blocking steer path. No path a client can reach still does an unscoped run lookup, every surface sets the ctx tenant, and the new tests fail without the fix. Same-tenant behaviour is preserved. +Concerns/Blockers: None blocking. Nits: WARN log volume, untested call sites in the consumer and the injection path, the owner-role change missing from the PR body, and a rebase onto the current dev. The shared index remains tracked in dewee#1008. diff --git a/plans/reports/code-reviewer-261011-2320-dewee-991-956-rereview.md b/plans/reports/code-reviewer-261011-2320-dewee-991-956-rereview.md new file mode 100644 index 0000000..fe586a0 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2320-dewee-991-956-rereview.md @@ -0,0 +1,89 @@ +# Re-review: dewee #991 (3bf114aa, 670c5870) and #956 (f0eee75a, 8042aeda, 45aa773d) + +Reviewer: isolated, read-only. Baselines: `code-reviewer-261011-2150-dewee-991-review.md` and `code-reviewer-261011-2140-dewee-956-957-rereview.md`. Implementer report: `fullstack-261011-2215-dewee-991-956-stack.md`. +Line numbers below refer to the 956 worktree (HEAD 45aa773d) unless noted otherwise. + +## Gates I ran (956 HEAD, a superset of 991) +- `go test ./internal/http ./internal/agent ./internal/gateway/... ./internal/channels -count=1` in the default, `sqlite` and `sqliteonly` modes. + - gateway, gateway/methods and channels pass. + - The http and agent failures are exactly the dewee#936 baseline list from the implementer report: 10 http tests, plus the 2 WorkspaceSetup tests in the sqlite modes, plus `TestGetForRunEvictsLeastRecentlyUsedPinnedLoops`. No other test fails. +- The focused new tests (sqlite tags) pass, and `go test ./tests/integration -run TestOpenAPI` passes. I ran `go clean -testcache` afterwards. +- `git range-diff bd177a2d..a078e857 670c5870..8042aeda`: 8042aeda is identical to a078e857. f0eee75a differs only in the runner/stream hunks that took #991's code. + +## 1. Earlier findings +| ID | Status | Evidence | +|---|---|---| +| S1 buffered stop | Fixed | `provisioning_chat_runner.go:86-91` wraps the error as `workspaceChatStoppedError` when `runCtx.Err()!=nil` (cancel is deferred, so only an abort or the fence sets it). `provisioning_chat.go:299-306` answers 409 `run_stopped` with run_id/session_key and skips `runFailed`. | +| S1 and the CCP | Compatible | `apps/dewee-app/src/lib/runtime-workspace-chat-client.ts:38-45` throws `AppError("runtime_workspace_chat_failed", error.message, 409)` for any non-2xx, and nothing retries on it. The CCP buffered call never sends `session_key` (:26-32), so the CCP cannot abort its own buffered run. 409 is only reachable through WS `chat.abort` or the fence. No CCP change is needed. | +| S2 double terminal | Fixed | `provisioning_chat_stream.go:146-151,186-202`. `stoppedSent` suppresses a second `chat.stopped`, and a held setup `chat.error` is dropped on a stop. Child run lifecycle events are not forwarded (`loop_pipeline_tool_callbacks_event_tap_test.go:53-60`), so a delegate's `run.cancelled` cannot end the parent stream. | +| S3 busy guard | Fixed | `provisioning_chat.go:257-262`, before `claimContinuation`, so a refused send never consumes a card. `router.go:365-368` uses `CompareAndDelete(sessionKey, runID)`, and `sessionRuns` stores runID strings (:344), so the comparison is valid. | +| S5 / #1001 | Fixed | `provisioning_chat_stream.go:91-109`: marshal happens outside the lock, then `done` is re-checked, the frame written and flushed under `mu`. The writes to `terminalSent`/`heldSetupError` in `onEvent` happen only for run-level terminal events, which fire on the Run goroutine, so they do not race with tool-goroutine taps. | +| S6 handler test | Fixed | `provisioning_chat_stop_test.go:118-180` drives the real handler, `NewAgentRouterWorkspaceChatRunner` and `agent.Router` with a request ctx cancelled mid-run. | +| N1 (956) | Fixed | `cmd/gateway_http_wiring.go:445-446` passes the registering runner and `d.agentRouter` as runtime. Busy guard `extension_api_chat.go:102`, abort `:195-198`. No run deadline exists; this matches `chat.send` and the implementer listed it as a concern. | +| N2 (956) | Fixed (doc) | The `rate_limit_rpm=0` coupling is documented in `docs/18-http-api.md` section 38 and in the OpenAPI `info.description`. The wiring is unchanged (`gateway_http_wiring.go:451-453`). | +| Nits 991 | Fixed | `channels.ExtractPayloadString` is exported (`channels/events.go:686`). Defer order is now drain, then cancel, then Unregister (`runner.go:61-63`, LIFO), matching chat.send. The fence is named in the comment. | +| Nits 956 | Fixed | Dead suspended branch removed. `entitlementSuspended` (`extension_api.go:211-219`, used at :263). `created_at` is omitted for nil/zero (`extension_api_read.go:323-326`, tag `omitempty` at :277). | + +## 2. Rebase correctness +- `provisioning_chat_runner.go:44-85` keeps #956's `traceName` default and `TraceName: traceName` (:84). It also keeps all of #991: WithoutCancel, InjectTeamDispatch, WithCancel, RegisterRun, the defers, `InjectCh`, `OnTraceCreated`, and the stop wrap. +- `provisioning_chat_stream.go` keeps #956's `RunContext/TraceName/Surface` (:63-70) and `streamWorkspaceChat` (:127). It also keeps #991's writer (`newSSEEventWriter(w, r.Context().Done())` at :135), `stoppedSent`, the stop branch, and the `channels.ExtractPayloadString` mapping (:239-241). +- Nothing was lost on either side. + +## 3. Stopped setup continuation that was not delivered +- How long: `setupContinuationClaimLease = 10*time.Minute` (`provisioning_chat_setup_turn_lease.go:28`). + - On a stop, `runFailed` is skipped (`stream.go:190`, `provisioning_chat.go:299-306`). + - Then `finish()` (`lease.go:130-140`) stops the heartbeat and leaves the mark when `!delivered`. + - A resend within 10 minutes gets 409 `continuation_sent` (`lease.go:167-170`). + - After that, `ReclaimStaleContinuation` plus `continuationInSession` re-sends the card. +- How often: rarely. `finalizeRun` flushes pending messages under `WithoutCancel` even on a cancel (`pipeline/pipeline.go:200,210-216`, `finalize_stage.go:115-117`), so most Stops fire `OnInputPersisted` and finalize the mark as delivered, which is correct. + - The undelivered case is a stop or fence before the user turn reaches the pending buffer (setup stage). + - It also covers a tenant-transfer fence, which cancels inside `RegisterRun` before `loop.Run` starts. +- Extension impact: it does not auto-retry a stop (an AbortError only dispatches `chat.stopped`, `use-chat.ts:205-207`). A manual re-click of the card (`ChatShell.tsx:122`) within 10 minutes, though, shows "The agent already continued after this card." That message is false, because the agent never saw the result. + - The 409 also sets `reconcile` (`use-chat.ts:211-212`). + - Its stream-failure retry (`retryContinuation`) has the same problem for a fenced run. +- Verdict: Should-fix (S-A below). The fix is one line and safe. + +## 4. Regressions +- chat.send: the only shared change is `UnregisterRun`'s `CompareAndDelete`, which is strictly more correct. Gateway/methods tests pass. +- Workspace chat (CCP stream): there is a new 409 busy refusal when the CCP resends into a session whose detached run is still going. That is the intended behavior. The CCP surfaces it as a generic `runtime_workspace_chat_stream_failed` 409. Nothing else changed for a completed or failed run. +- Extension routes: status, abort and busy now work for real. The read routes are unchanged apart from `created_at`. I found no regression. + +## 5. Do the tests fail without the fixes? +- `TestUnregisterRunKeepsNewerSessionRun` fails with `Delete`. +- `...StreamRunSurvivesRequestCancel` fails with a request-bound writer or runner. +- `...StoppedSetupTurnReportsStopOnce` fails if the old code emits `setup_result_delivered`. +- `...RefusesChatInBusySession` fails without the guard. +- The runner test's `workspaceChatStopped` assertion fails without the wrap. +- The implementer showed that the SSE concurrency test fails without `mu`. +- `TestExtensionAPIStreamRunIsRegisteredWithRouter` (`extension_api_router_sqlite_test.go:38-111`) is still meaningful: + - It exercises everything except the loop: the real route, auth, the registering runner, the real `agent.Router` (RegisterRun, IsSessionBusy, SessionRunID, AbortRun), the stop wrap and the SSE stop path. + - It would fail on the pre-rebase 956 wiring (no registration, so status is idle and abort returns false). + - Gap: it cannot prove that the real `loop.Run` emits `run.cancelled`. That is covered by `loop_run.go:219-220` and does not matter here, because the fallback `chat.stopped` at `stream.go:200-201` covers a missing event. +- `TestProvisioningHandlerBufferedChatStopIsNotRetryable` uses a fake runner that returns the stop error directly. That is acceptable because the runner-level test covers the wrap. + +## 6. Entitlement mapping and created_at +- Entitlement: correct. `RequireActiveTenant` wraps `ErrTenantEntitlementInactive` for any non-active entitlement (`tenantaccess/evaluator.go:72-73`). Only an exact `suspended` maps to `membership_suspended`. + - Both the PG and SQLite CHECK constraints allow only active/suspended (`migrations/000082...up.sql:3`), so the fallback is defensive. The lack of a test is acceptable. +- `created_at`: the extension accepts an omitted value. `historyMessage.created_at: nullableString` = `z.string().nullish()` (`src/api/schemas.ts:8,60`). `chat-state.ts:78` uses `m.created_at ?? ''` only in the id fallback, and the gateway always sends `id`. Tests already build `created_at: undefined` (`tests/unit/chat/history.test.ts:6`). + +## Findings + +### Blocking +None. + +### Should-fix +- **S-A. A stopped continuation that was not delivered stays locked for 10 minutes with a false message.** + - Where: `provisioning_chat_stream.go:188-192` and `provisioning_chat.go:299-306`. + - Evidence: section 3. + - Fix: on a stop, still settle the lease without reporting a failure, for example call `setupTurn.runFailed(inputDelivered)` and ignore its result. With `inSession=false` it releases the mark (the run left no trace, so a resend is safe). With `true` it is a no-op, and `finish()` finalizes. + - Add a case to `TestProvisionWorkspaceChatStoppedSetupTurnReportsStopOnce` that stops without `OnInputDelivered`, then resends the card and expects a run, not a 409. + +### Nit +- **N-a.** A tenant-transfer fence is reported to the person as a user stop (`run_stopped` / `chat.stopped`) with no explanation (`runner.go:86-91`). It is rare. Consider a distinct log line or code. +- **N-b.** `entitlementSuspended` adds a second `GetTenant` on the refusal path (`extension_api.go:211-219`). The status is already in the wrapped error text (`evaluator.go:73`). This is cosmetic. +- **N-c.** The busy guard is check-then-act: two simultaneous sends can both pass `IsSessionBusy` (`provisioning_chat.go:259`, `extension_api_chat.go:102`). This is the same as other surfaces, and the narrow window makes it acceptable. +- **N-d.** `CompareAndDelete` fixes the "old run ends last" ordering only. If the newer run ends first, the index is cleared while the older run is still running. That can no longer happen on these routes because of the busy guards, so this is informational. + +Status: DONE +Summary: Every earlier finding (S1, S2, S3, S5/#1001, S6, N1, N2 and the nits) is fixed correctly. The rebase lost nothing, CCP and extension clients stay compatible, and the tests fail without the fixes and pass now, apart from the known Windows baseline failures. One should-fix: a stopped continuation that never reached the session stays locked for 10 minutes, and a resend in that window gets a false "already continued" 409. +Concerns/Blockers: no -race (no cgo). The 991 branch was not tested on its own; the 956 HEAD contains it. diff --git a/plans/reports/code-reviewer-261011-2325-dewee-957-992-rereview.md b/plans/reports/code-reviewer-261011-2325-dewee-957-992-rereview.md new file mode 100644 index 0000000..e457df0 --- /dev/null +++ b/plans/reports/code-reviewer-261011-2325-dewee-957-992-rereview.md @@ -0,0 +1,80 @@ +# Re-review: dewee 957 fix 7948bfe3 + 992 fix 3dd36e5a + +Isolated, read-only review. Baselines: `code-reviewer-261011-2140-dewee-956-957-rereview.md` (N3) and `code-reviewer-261011-2230-dewee-992-993-review.md` (agent_key). + +Runs: +- 957, vitest `src/lib/extension-api/`: 39/39 pass. +- 992, vitest `src/lib/extension-api/` + `runtime-sessions-client.test.ts`: 50/50 pass. `tsc --noEmit`: exit 0. +- 992, `go test ./internal/http -run 'ProvisioningHandlerSessions|ProvisioningWorkspaceSessions'`: ok. The same with `-tags sqlite` (incl. `AgentKeyUnderscoreIsLiteral`): ok. +- `go test -tags sqlite ./internal/store/sqlitestore -run Session`, `go test ./internal/store/pg -run Session`, `./internal/gateway/methods -run Session`: ok. +- `go test -tags sqlite ./internal/http -run 'Session|ChannelInstance'`: one failure, `TestProvisioningHandlerStreamContinuesExistingSessionWithMedia`. The cause is a Windows path inside a raw JSON literal (`invalid character 'U' in string escape`). It is unrelated to this diff and is tracked in dewee#936. +- `go clean -testcache` was run afterwards. +- "Fails without the fix" was established by reasoning, not by running against the parent commit, because the worktrees stay unmodified. + +## 1. 7948bfe3: relay single terminal frame (N3) + +**N3 is closed.** `extension-chat-relay.ts:143` now runs `if (!terminated) writeError(error)`. +- `terminated` is set in `write` (`:80`) for every terminal event, including relayed runtime frames, `chat.stopped` (`:125,:138`) and `writeError` itself. +- Every terminal path goes through `write`, so no path can emit two terminal frames. +- **The test fails without the fix.** `onCompleted` throws synchronously, before `.catch` attaches (`:133`). The error reaches the catch after `message.completed` was written. At the parent this appends `chat.error`, and `toEqual(["event: message.completed"])` fails. The test's second pull (`controller.error`) is never reached, because the loop breaks on the terminal event. It is harmless. + +**The overall 957 branch (origin/dev...7948bfe3, 3 commits, `apps/dewee-app` + 2 docs only) has no unresolved blocking finding.** +- N1 and N2 are Gateway (Go) findings for 956. They are not in this diff. +- B1, B2, S1-S4, S6 and S7 were already closed. +- N4 (sessions fan-out) and S5 are replaced by #992. #992 is stacked on 7948bfe3, and 3dd36e5a's parent chain contains it. + +### Blocking +None. +### Should-fix +- **Merge order.** The 957 branch alone still ships the 10-call fan-out (`apps/dewee-app/src/pages/api/extension/v1/workspaces/[workspaceId]/sessions.ts` at 7948bfe3, N4/S5). Land #992 with 957, or immediately after it. Do not release 957 by itself. +### Nit +- `extension-chat-relay.ts:141-143`: after a terminal frame, the caught error is dropped without a log. A synchronously throwing audit hook (`onCompleted`) or mark-ready failure then vanishes silently. Consider a server-side log of the error name (never the payload). +- The re-review nits that were already accepted are still open and unchanged: CORS `*` entry, workspaces N+1, `created_at` parity, refresh loser pair. + +## 2. 3dd36e5a: NormalizeAgentID keys + LIKE escaping (992) + +**The finding is fixed.** +- The Gateway uses `config.IsValidAgentID` (`internal/http/provisioning_sessions.go:118`), which is the same `validIDRe` as `NormalizeAgentID` (`internal/config/normalize.go:11,53`). +- The BFF regex (`sessions.ts:24`, `/^[a-z0-9][a-z0-9_-]{0,63}$/`) is identical. +- Neither regex is multiline, and `$` anchors at the end of input in both JS and Go. +- Both sides trim before validating. + +**The LIKE escaping is correct.** +- PG (`internal/store/pg/sessions_list.go:39,45`): + - The Go literal `"ESCAPE '\\'"` produces the SQL `ESCAPE '\'`, the same form as `pg/channel_contacts.go:85` and `pg/tracing.go:283`. + - The values go through `pg.escapeLike` (`tracing.go:337`), which escapes `\`, `%` and `_` in a single pass, so `\` is handled correctly. + - PG's default LIKE escape is already `\`, so the clause is explicit rather than required. +- SQLite (`internal/store/sqlitestore/sessions_list.go`, AgentID/Channel conditions): + - SQLite has no default escape, so the clause is required here. + - `'\'` is a single backslash, because SQLite literals have no backslash escapes. + - `sqlitestore.escapeLike` (`memory_search.go:128`) replaces `\` first, then `%` and `_`. This is correct. +- Every value is still a bound parameter. + +**The tests fail without the fix:** +- At the parent, the Go accept test and the TS `sales_bot` test get 400 or an empty list. +- In the sqlite test, without the escape, `agent:sales_bot:%` also matches `agent:salesxbot:...`, so total=2 and the test fails. This test does check the escape itself, not just the validation. + +**Docs.** `docs/ccp-session-detail-inspection.md:17` and `docs/extension-oauth-and-bff.md:96` are accurate. No stale "slug" wording remains in `docs/` or `apps/dewee-app/src`. + +**Regressions.** +- `buildSessionFilter` is shared by `methods/sessions.go`, `http/sessions.go`, `channel_instances_verify.go:274`, `usage.go` and the per-agent count. +- Escaping makes `_` and `%` literal for all of these callers. This is a correctness gain: for example, a channel instance named `my_bot` no longer matches `myxbot`, and a client-supplied `%` no longer acts as a wildcard (it was tenant-scoped anyway). +- No caller relies on wildcard semantics. +- The broader session test suites pass. + +### Blocking +None. +### Should-fix +None. +### Nit +- **Keys of 65-100 characters.** + - `internal/http/agents.go:247` creates agents with `isValidSlug`, which has no length cap. The column is `VARCHAR(100)` (`sqlitestore/schema.sql:251`). + - Such a key is now refused: 400 at `provisioning_sessions.go:118`, and an empty list at `sessions.ts:35`. The previous slug check accepted it. + - This is an edge case: CCP-created agents use `NormalizeAgentID` (64 characters or fewer). + - Accept and document it, or cap `isValidSlug` agent keys at 64. +- **No PG-level test of the escape.** Only SQLite exercises it. `buildSessionFilter` in PG is pure, so a cheap check would assert the `ESCAPE` clause and the `agent:sales\_bot:%` argument (next to `pg/sessions_list_hidden_test.go`). +- **The PG `ESCAPE '\'` assumes `standard_conforming_strings=on`.** That is the default and the existing repo convention, so this is informational only. + +Status: DONE +Summary: N3 is fixed correctly with a test that fails at the parent. The 957 branch has no unresolved blocking findings (N1/N2 are 956 Gateway findings, N4/S5 move to #992). The 992 fix accepts NormalizeAgentID keys on both sides with identical patterns, and it escapes LIKE correctly on PG and SQLite with a test that checks the escape. +Concerns/Blockers: Land #992 together with 957, or right after it, so the sessions fan-out does not ship alone. One sqlite http test fails for a known Windows reason (dewee#936). It is unrelated to this diff. diff --git a/plans/reports/code-reviewer-261011-2330-dewee-958-rereview.md b/plans/reports/code-reviewer-261011-2330-dewee-958-rereview.md new file mode 100644 index 0000000..9f4feec --- /dev/null +++ b/plans/reports/code-reviewer-261011-2330-dewee-958-rereview.md @@ -0,0 +1,79 @@ +# Security re-review: dewee#958 fix commit 3b98e0c6 + +I re-reviewed `feat/958-credential-request-capability` (bd177a2d..3b98e0c6) in isolation and did not write this code. Paths are relative to `D:/www/nextlevelbuilder/worktrees/dewee-958-credential-request-capability`. Nothing was edited; `git status` is clean. + +## Verdict +**Ready to merge from a security standpoint. Nothing is blocking.** Acceptance scenario D holds: +- A non-setup agent is refused unless it has an enabled policy, and only for allowlisted targets. The policy is re-read at call time and again at respond. +- Duplicate requests return the open card. +- Revoked, narrowed or expired cards are refused or cancelled. +- The agent only ever receives an action id or a `credential_ref`. + +## How I checked +- Focused tests pass: + - `internal/http` with `-tags sqlite` and `-tags sqliteonly` (`CredentialRequest|CredentialPolicy`); + - `internal/tools`, `internal/store`, `internal/gateway/methods`; + - `internal/store/sqlitestore` with `-tags sqliteonly` (`WorkspaceSetup`). +- I did not re-run PostgreSQL; the implementer's PG run is the only evidence for it. +- **Mutation checks.** I disabled each fix with `go test -overlay` using copies in the scratchpad, so the worktree was untouched. Every mutation made a test fail: + - m1, no respond re-check (`workspace_setup_respond_core.go:168`): `TestCredentialRequestLifecycleSQLite` fails. + - m2, caps disabled (`credential_request_tool.go:259`): the `...CapsOpenCardsPerConversation` and `...PerAgent` tests fail. + - m3, step key made unique per call (`credential_request_cards.go:54`): `...ReturnsTheOpenCardForADuplicate` and `...IgnoresAnAgentChosenStepID` fail. + - m4, HTTP `requireTenantAdmin` removed (`agents_credential_request_policy.go:110`): `...RefusesAMemberHoldingAnAdminScopedKey` and `...WithoutATenantStoreRefuses` fail. + - m5, sweep turned into a no-op (`workspace_setup_credential_request.go:79`): the lifecycle sweep case fails. + - m6, MCP match always true (`agent_credential_request_policy.go:202`): `...ScopesMCPCardsToTheNamedServer` fails. + - m7, WS membership check removed (`agents_credential_policy.go:97`): `TestWSCredentialPolicyRefusesAMemberWithTheAdminRole` fails. +- S1 is covered by `credReqCardNeverMovesAPlanStep`. It forges `RequestedBy` with plan step id `provider.openrouter`, so it fails if the marker guard at `workspace_setup_respond_appliers.go:235` is removed. +- `go clean -testcache` was run afterwards. + +## Prior findings +| ID | Status | Evidence | +|---|---|---| +| B1 | Fixed | The re-check runs before the claim (`respond_core.go:168`) and goes through `CredentialRequestCardDenial` (`credential_request_target.go:172`), which fails closed if the agent is missing, belongs to another tenant, has no policy, or the marker does not match `agent_id`. The sweep is wired for HTTP (`agents_credential_request_policy.go:215`) and WS (`agents_credential_policy.go:215`). The TTL is 1h (`credential_request_tool.go:221`). | +| B2 | Fixed | The dedup key is server-derived (`credential_request_cards.go:52`) and the agent's `step_id` is ignored. The caps are 3 per session and 10 per agent (`credential_request_tool.go:243-263`). Expired pending cards are excluded from the count (pg `workspace_setup_actions.go:201`, sqlite `:245`). | +| S1 | Fixed | `stepProgressAfter` returns nil when the card carries the marker or the `credreq/` prefix (`workspace_setup_respond_appliers.go:235`). | +| S2 | Fixed | HTTP routes check `requireTenantAdmin` (`agents_credential_request_policy.go:110`); WS checks `canManageTenantSettingsWS` (`agents_credential_policy.go:97`). A nil store refuses. | +| S3 | Fixed | The policy needs exactly one of `server_id` (canonical uuid) or `server_name` (`agent_credential_request_policy.go:161-175`). The card's server is resolved through the tenant-scoped `GetServer` (`pg/mcp_servers.go:80-86`), and this runs both at create and at respond. | +| S4 | Fixed (documented) | `docs/workspace-setup-onboarding.md:135`. | +| S5 | Fixed | The real tool, store and respond path run for a non-setup agent, with no secret on any surface. The suite also covers revoke, the sweep, the step collision, duplicates, caps, the role checks, and a PG mirror. | +| S6 | Fixed | The docs (lines 100-135), the CLI help and OpenAPI describe the role check, the 409 details, the TTL, the caps and MCP scoping. | +| N1 | Fixed | WS broadcasts `EventCacheInvalidate` (`agents_credential_policy.go:205-210`). | + +## The implementer's concerns, by real risk +1. **The caps are soft. Low risk; acceptable.** A race needs parallel tool calls, and every racing call must still pass the allowlist. Within one session each distinct key (type, identifier, mode) gets at most one open card, because the `(tenant, session_key, step_id)` unique index covers the server-derived key. Overshoot is therefore bounded by the size of the allowlist, at most 32×2 keys, and the cards are only prompts that need a person. A transactional count or an advisory lock is not worth it now. +2. **The gap between re-check and claim is real but tiny. Low risk.** The sweep lists only `pending` cards (`workspace_setup_credential_request.go:76`). A policy write that commits after the re-check (`:168`) but before `ClaimAction` (`:173`), with its sweep running after the claim, lets that one answer apply. The window is microseconds, and it needs an admin answering at the same moment another admin revokes. A cheap hardening (Nit N2 below) is to re-run the denial after the claim and release or cancel the card on refusal. A stale `applying` claim swept back to `pending` is re-checked on the next respond. +3. **A shared dedup key for oauth_request and secret_request. Not a security issue.** The second call returns the first card, which is the same allowed target. This only affects function; see N1. +4. **Admin keys with no owner user are now refused. Correct, and consistent with respond.** Respond already requires `requireTenantAdmin`. Writes already needed a person, so the only behaviour lost is GET for ownerless keys. A system owner (`IsOwnerRole`) still bypasses the membership check, but writes stay person-only through `RequireCredentialPolicyEditor`. + +## Can the agent reach the secret? No path found +- **Tool result.** `ForLLM` holds only the action id (`credential_request_tool.go:232-235`). `Data` is the envelope: payload, fields and target, never values. +- **Continuation and transcript.** `setupContinuation` sends `a.Result` (`workspace_setup_continuation.go:27-35`), which is a `SecretResult` or `OAuthResult` holding a `credential_ref`. A cancelled or refused card has no result and gets no continuation. +- **Logs, activity and spans.** They hold ids, kind, target type, identifier, mode, server name and denial reason (`credential_request_tool.go:267-312`, `workspace_setup_credential_request.go:36,52`). The lifecycle test asserts a sentinel is absent from tool results, card rows and the activity log. +- **Residual risk.** `cli_agent_credential` is documented as the remaining exposure (S4). + +## Tenant isolation: holds +- The tool compares the agent's tenant with the context tenant (`:167`), and the re-check compares the agent's tenant with the card's (`credential_request_target.go:184`). +- `ListOpenActionsByRequester` adds `tenant_id` (pg `:200`). +- MCP, provider and channel lookups are tenant-scoped. +- The policy routes return 404 for another tenant's agent (`agents_credential_request_policy.go:129`), and the sweep refuses `uuid.Nil` (`workspace_setup_credential_request.go:64`). + +## Migrations and the RequestedBy marker: none needed, and the marker is reliable +- Only the tool writes `credential_request:` (`credential_request_tool.go:216`). `fillSetupActionRunLinks` keeps a requester the caller already set and otherwise writes `agent:` (`credentialed_exec_setup_policy_card.go:72-78`). The other writers are the concierge (user id or `system`, `workspace_setup_concierge_routes.go:144-149`), the setup policy and `ui_action` (`agent:`). None of them can produce the prefix. +- If a non-tool card did carry the prefix (for example a user id that literally begins with it), the re-check would fail closed: it would be refused or cancelled, never widened. +- A tool card cannot lose the marker: no update path rewrites `requested_by`. If `agent_id` disagrees with the marker, the card is denied (`credential_request_cards.go:43-45`). + +## Blocking +None. + +## Should-fix +None. + +## Nit +- **N1. The dedup key leaves out the record id of an update card** (`credential_request_cards.go:52-55`; the identifier is the provider or channel *type*, see `credential_request_target.go:101-104`). Two update requests in one session, for two different Telegram instances or two providers of the same type, collapse into the first card. The agent is told the card for instance B "is already open", and the person rotates instance A. This is not an escalation, because both were allowed, but it is wrong. Add `tg.ID` to the key for `mode=update`. +- **N2. Re-check after `ClaimAction` as well** (`respond_core.go:173`): on a denial, cancel the card and refuse. This closes the narrow window described in concern 2. +- **N3. `CredentialRequestCardDenial` ignores the agent's status** (`credential_request_target.go:183-190`). Cards of a disabled or archived agent stay answerable for up to 1h. Consider denying when the status is not active. +- **N4. `server_name` matching follows renames.** A server later renamed to the allowlisted name (`agent_credential_request_policy.go:202`) becomes requestable. This is admin-controlled, so it is low risk; recommend `server_id` in the docs. + +Status: DONE +Summary: All of B1, B2, S1-S6 and N1 are fixed and tested; mutation checks show each fix is guarded by a failing test. There is no secret path to the agent, tenant isolation holds, and the RequestedBy marker is reliable without a migration. +Concerns/Blockers: None blocking. Nits: the dedup key ignores the update record id (N1), an optional post-claim re-check (N2), agent status ignored at re-check (N3). PG was not re-run by me. diff --git a/plans/reports/code-reviewer-261012-0020-extension-final-review.md b/plans/reports/code-reviewer-261012-0020-extension-final-review.md new file mode 100644 index 0000000..cc3ff46 --- /dev/null +++ b/plans/reports/code-reviewer-261012-0020-extension-final-review.md @@ -0,0 +1,104 @@ +# Extension final review: stream errors, refusal reasons, credentials form, live e2e + +Scope: src/api/dewee-client.ts, src/api/errors.ts, src/credentials/CredentialsForm.tsx, src/shared/i18n.ts, +tests/e2e/live-*.spec.ts, tests/e2e/support/{live.ts,stub-llm.mjs}, tests/unit/{api,credentials}, docs/api-contract.md. +Also read: use-chat.ts, chat-state.ts, ChatShell.tsx (respond), ActionCard.tsx, client-factory.ts, sse-reader.ts. +Method: reading only. The Bash tool failed on every command, so lint, typecheck, tests and build were NOT run. + +## Blocking + +None. No secret reaches storage, logs, URLs or chat. No agent HTML is rendered, and there is no remote code. +The no-auto-resend invariant holds: `retry` only runs from a user click (use-chat.ts:285-291). + +## Should-fix + +1. Retrying after a real pre-send failure is lost in an existing conversation. Files: use-chat.ts:210-212, chat-state.ts:194-195, use-chat.ts:101-111. + `attempt()` (dewee-client.ts:198) and `authorization()` failures (client-factory.ts:20, `background_unavailable`) + both return status 0, the same status as a mid-stream drop. When a session key exists, `reconcileNeeded` is true, so + `retryText` is removed. Scenario: the person is offline and sends in an existing chat. The panel shows + "Reconnecting" for up to about 3 minutes (90 polls; a status-0 poll does not break the loop at line 106), `loadSession` + fails, and the message has no "Send again" button. If the network comes back during the reconcile, the history reload + replaces the optimistic user turn, and the unsent message disappears without notice. + Fix: after reconcile, compare the fresh history with the text that was sent. If the turn is missing, restore it with + `retryText`/`retryContinuation`. Also stop polling when the first status poll returns status 0 and `navigator.onLine` + is false. Add a use-chat test for both branches; no hook-level test exists (tests/unit/chat has only reducer tests). +2. In a new chat, a drop before `chat.started` offers "Send again" even though the server accepted the request. + File: dewee-client.ts:139-148 with use-chat.ts:212. A 200 response proves the Gateway accepted the turn. If the body + breaks before the first frame, `accepted` is false and there is no session key, so `reconcile` is false and the + retry button appears. Clicking it starts a second run in a second session. + Fix: mark errors raised after the response arrived (for example `new DeweeApiError(0,'network_error',…)` plus + a `afterResponse: true` field). In use-chat, do not offer a retry for those errors. Tell the person the reply may + continue in the sessions list. +3. The new catch-all also relabels non-transport failures as "connection lost". File: dewee-client.ts:148. + Any non-`DeweeApiError` error (a bug in `mapRuntimeFrame`, a decoder error, or a raw throw from a future + auth adapter) becomes `network_error` and starts a reconcile. Fix: narrow the check to `error instanceof TypeError` + and rethrow other errors unchanged. +4. A bad `details` value makes the client drop the whole error envelope. File: errors.ts:40. + `details` must be an object with `reason` as a string of at most 80 characters. If an endpoint sends + `"details": null` (Go `Details any` holding a typed nil map), a non-string `reason`, or a reason longer than 80 + characters, `safeParse` fails. The client then drops the server `code`, `message` and `retry_after_seconds` and falls + back to `conflict`, `request_failed` or `HTTP 409`. The claim that unknown reasons are ignored only holds for short + string reasons. Fix: `details: z.unknown().optional()`, then read the reason with + `z.object({ reason: z.string() }).safeParse(details)` and check it against `specificReasons`. Add tests for + `details: null` and `{ reason: 5 }`. +5. Typed secrets stay in memory after the card closes without a submit. File: CredentialsForm.tsx:20, 43-49. + The card can close while the form is still mounted, for example because `isUiActionOpen` is evaluated again after + expiry on a parent re-render, or because another device answers and the card reloads. The inputs disappear, but + `values` still holds the secret until the component unmounts. Nothing is written to the DOM or storage, but this + breaks the rule that "the panel never holds them again". Fix: `useEffect(() => { if (!interactive) { setValues({}); + setMissing([]); } }, [interactive])`. In CredentialsForm.test.tsx:109-124, also check + `container.innerHTML` does not contain SECRET, and add a test that types a value, re-renders with + `interactive={false}`, then re-renders with `true` and expects an empty input. +6. The live helpers send the admin master token and run destructive calls against any host. Files: tests/e2e/support/live.ts:90-116, + live-credentials.spec.ts:43-45 and 115-118. The specs skip cleanly in CI, because the file-level `test.skip` + needs both env vars. But if a developer's `.env` points `E2E_GATEWAY_URL` at a shared or production Gateway, + the run uses the master token with `X-GoClaw-User-Id: system` to delete agent policies, mint and revoke API keys, + and run `sweep`/`expire` on whatever `E2E_DB_DSN` names. Fix: in `requireLiveGateway`, skip unless the hostname + is loopback (`localhost`, `127.0.0.1`, `::1`) or `E2E_ALLOW_REMOTE=1` is set. Apply the same check to `E2E_CLOUD_ORIGIN`. +7. The revoke e2e test does not prove the new reason path. File: live-credentials.spec.ts:242-245. It also accepts + "This request is already closed.", so a Gateway that never sends `credential_policy_revoked`, or a broken + `errors.ts` mapping, still passes. Fix: once dewee#1019 is final, assert only the revoked text, or record the + generic fallback as a known gap. +8. The docs and the code disagree about the error shape. File: docs/api-contract.md:178 says the respond call + "returns `409 credential_policy_revoked`", which reads as `error.code`. The client and its unit test + (errors.test.ts:32) expect `code: 'conflict'` with `details.reason`. Fix: state the actual envelope (code plus + `details.reason`) after checking dewee#1019. The local dewee `dev` branch has no `credential_policy_revoked`, so I + could not check this claim. + +## Nits + +- live-selfhosted.spec.ts:117-139: a failed assertion skips the revoke of the minted key (`created.key`). Move the + revoke into `finally`. +- live.ts:22-25: the non-admin skip does not require `E2E_WORKSPACE_ID`, but `api()`, `history()` and + `rememberedSession()` use it. Without it, the selfhosted and resilience specs fail on `/workspaces//…` instead of + skipping. Require it there too. +- live-resilience.spec.ts:36: the relay ignores https and defaults to port 80. Document that the relay supports only + http on loopback. +- dewee-client.ts:148 creates the message "The connection was lost", but the UI shows `error.network_error` + ("Cannot reach the server"), which is misleading mid-reply. Consider a separate `connection_lost` code with its own en/vi strings. +- live-credentials.spec.ts:62-68: `watchLeaks` excludes respond requests by URL but never checks that the respond + URL does not contain the sentinel. +- sse-reader.ts:14: the line buffer is unbounded, so a server that never sends a newline can grow memory + without limit. Open a separate `security` issue for this rather than widening this PR. + +## Verified OK + +- Mid-stream drop: when a session key exists or `chat.started` arrived, `retryText` is removed and the panel + reconciles. The unit test at dewee-client.test.ts:301-321 covers the client mapping. +- A user abort is passed through as `AbortError` (`signal.aborted` guard). A stall maps to `stream_stalled`. +- errors.ts: only `credential_policy_revoked` is promoted. An unknown string reason keeps the server code (tested). +- i18n: the en and vi strings for `error.credential_policy_revoked` exist (i18n.ts:88, 265). They render as text. +- CredentialsForm: values are cleared before the respond call is awaited. The refusal alert renders from + `error` state, never from agent text. No input is rendered when the form is not interactive. +- Live specs: the file-level `test.skip` runs before any network call. Nothing writes keys, tokens or sentinels to the + console. Screenshots are taken before fill or after the form is cleared. stub-llm binds to 127.0.0.1 and logs only + its URL. `.env` is gitignored. +- Backend status: GET /v1/auth/token/status and the UI action stack are present on the local dewee `dev`. I could not + check the PR numbers or states (no gh access). + +Status: DONE_WITH_CONCERNS +Summary: No blocking security defects. There is no auto-resend anywhere. However, a real pre-send failure in an +existing chat loses the retry option (and can silently drop the message), and a drop before `chat.started` in a new +chat still offers a retry that duplicates the run. +Concerns/Blockers: lint, typecheck, tests and build were not run (the shell was broken). The dewee#1019 error shape +and the PR states in the Backend status table are unverified. diff --git a/plans/reports/code-reviewer-261012-0100-extension-rereview.md b/plans/reports/code-reviewer-261012-0100-extension-rereview.md new file mode 100644 index 0000000..c76442a --- /dev/null +++ b/plans/reports/code-reviewer-261012-0100-extension-rereview.md @@ -0,0 +1,101 @@ +# Extension re-review after the review fixes + +Method: I only read the code. Bash failed with ENAMETOOLONG on every command, so lint, typecheck, tests and the build were NOT run. I did not open `.env`. + +## Status of the earlier findings + +| # | Status | Evidence | +|---|---|---| +| S1 pre-send vs post-response | Fixed, with gaps (see B-1..B-3) | use-chat.ts:263-291 and 136-143; errors.ts:18; dewee-client.ts:141-162; use-chat.test.tsx:119-268 | +| S2 new chat drops after 200 | Fixed | use-chat.ts:269-272; use-chat.test.tsx:169-184 | +| S3 narrower catch-all | Fixed, but it causes a regression (B-4) | dewee-client.ts:149-155, 219-222; dewee-client.test.ts:344-363 | +| S4 lenient `details` | Fixed (N-1 remains) | errors.ts:47, 51, 84-85; errors.test.ts:38-44 | +| S5 CredentialsForm clears values | Fixed | CredentialsForm.tsx:30-33; CredentialsForm.test.tsx:124, 127-145 | +| S6 live host guard | Fixed. `E2E_DB_DSN` is still not checked; the fix report says so | live-host-guard.ts:6-25; live.ts:26-31, 96-99; live-cloud.spec.ts:15-16 | +| S7 revoke e2e asserts the exact reason | Fixed | live-credentials.spec.ts:245 | +| S8 docs | Fixed. architecture.md:40 is still incomplete (B-5) | api-contract.md:178; development.md:26 | +| Nits | 4 of 4 claimed fixes verified | live-selfhosted.spec.ts:144-147; live.ts:27; i18n.ts:100-101 and 280-281; live-credentials.spec.ts:65, 102 | + +Two nits from the earlier review are still open. The fix report does not claim them: +- the relay supports only http (live-resilience.spec.ts:36); +- the SSE buffer is unbounded (deferred to #2). + +New codes: +- `connection_lost` and `delivery_unconfirmed` have en and vi strings (i18n.ts:100-101, 280-281), and so does `chat.offlineWaiting` (:66, :246). +- Errors render inside `role="alert"` (MessageList.tsx:23). The offline notice renders inside `role="status"` (:122). +- Leaving them out of `RecoveryCode` is correct. That union lists server `chat.error` codes only, and `ChatMessage.error.code` is `RecoveryCode | string`. +- The doc comment at errors.ts:3 that lists client codes is stale. + +## Blocking + +None. No secret reaches storage, logs or URLs. The panel never resends automatically: `retry` runs only on a click (use-chat.ts:351-357). + +## Should-fix + +**B-1. A nested reconcile is overwritten by the outer `idle`, so the panel shows idle while a run is live.** +- Where: use-chat.ts:110 with 155-158. +- How it happens: the 90 polls run out after about 3 minutes, which a long tool run can exceed. The run is still going, so `loadSession` sees `status.running` and starts a second reconcile, which dispatches `reconciling`. The outer reconcile then dispatches `idle`. +- Failure: the composer opens while the run continues. A send gets a 409 and starts a third reconcile. If the user sends after the run ends, the nested reconcile's later `history` dispatch replaces the messages in the middle of the new stream. The optimistic turn and the streaming reply disappear, and `patchStreaming` drops every later delta. An accepted turn vanishes from view. +- Fix: + - Give `loadSession` a `{ followRun: false }` option for calls from reconcile. + - Have reconcile return or keep looping while `status.running`, and dispatch `idle` only from the reconcile that owns the loop. + - Add a test: status stays `running` for more than 90 polls (fake timers). + +**B-2. `endsWithUserTurn` matches on text alone, so a turn can be lost or shown twice.** +- Where: use-chat.ts:58-61 and 157. +- Lost: the person sends "continue", gets a reply, then sends "continue" again while the connection is flaky (network_error in an existing chat). The earlier "continue" is the last user turn in history, so the new turn is treated as delivered and silently dropped. +- Shown twice: another tab or device posts a later turn, or the server stores the text in a slightly different form. The last user turn then differs, so `restoreTurn` appends a duplicate and offers "Send again" for a turn the server already has. +- Fix: + - Record a server baseline before the send: the history length, or the last server message id (`m`) from the last `history` action. + - Treat the turn as delivered only if a new user message after that baseline equals the text. + - Add a test that repeats the previous text. + +**B-3. When history fails to load, the turn is still restored with "Send again", although nothing proved it missing.** +- Where: use-chat.ts:155-157 (`history === null` means `endsWithUserTurn` is false). +- Failure: the network flaps. Status polls fail with status 0 until the poll cap. Then `loadSession` fails, and a turn the server may already hold gets a retry button. One click starts a duplicate run. +- Fix: + - When `history` is null, do not dispatch `restoreTurn` with `retryText`. + - Show `delivery_unconfirmed` with the existing reload link instead. + - Note that the `restoreTurn` branch for an existing `localId` (chat-state.ts:210-211) only runs in this case and has no test. + +**B-4. A non-network error after a 200 now offers "Send again", which duplicates the run.** +- Where: dewee-client.ts:154 and use-chat.ts:257-291. +- How it happens: the S3 fix rethrows non-`TypeError` body errors unchanged, and they carry no `afterResponse` mark. For example, a `RangeError`, a decoder or `mapRuntimeFrame` bug, or a reader error before `chat.started`. +- Failure: in use-chat, `apiError` is null, so `delivered = accepted = false` and `unsure` is false. The else branch offers `retryText` in both new and existing chats, although the 200 proved the server accepted the turn. +- Fix: in the client, keep the original error but mark acceptance. For example, `if (responded && !(error instanceof DeweeApiError) && !isAbort(error)) throw Object.assign(error, { afterResponse: true })`, or wrap it in `DeweeApiError(0,'runtime_request_failed',…,true)` with `cause`. Alternatively, in use-chat, treat any non-API error as not provably undelivered. Add a hook test. + +**B-5. A new chat still offers "Send again" after failures where the request may have reached the server.** +- Where: use-chat.ts:267-276. The failures are a stall before the response, or a rejected fetch. +- Failure: a `stream_stalled` with `afterResponse: false` fires after 120 s with no headers. The request was almost certainly sent, so "Send again" can create a second session with the same run. +- This breaks the stated invariant that "Send again" appears only when the server provably did not get the message. It also contradicts architecture.md:40, which says only failures that "never left the panel" keep "Send again". +- Fix: + - For a new chat, map `stream_stalled` to `delivery_unconfirmed` with no retry. + - Keep the retry for `network_error` only when `navigator.onLine === false` at the time of failure. + - If you choose to accept the risk instead, that is a product decision: record it in architecture.md. + +**B-6. A card continuation lost before delivery is never retried.** +- Where: use-chat.ts:275 (`text !== undefined` only) with chat-state.ts:198. +- Failure: the person saves a credential, the continuation stream gets `network_error` in a known session, and the panel reconciles with no `unconfirmed`. Setup-turn markers never appear in history (api-contract.md:92), so nothing restores the continuation, and `retryContinuation` is cleared. The card is already resolved, so the agent never continues and no button can resume it. +- Fix: the Gateway delivers each `setup_action_id` at most once (api-contract.md:152), so a resend is safe. After reconcile, keep `retryContinuation` for continuation failures that are not provably delivered. Add a test. + +## Nits + +- **N-1. errors.ts:43-45.** `retry_after_seconds` must be a positive integer of at most 3600, and the message at most 500 characters. A value of 0, a float or an overlong message still drops the whole envelope, and with it `details.reason` (the same class of bug as S4). Fix: use `.catch(undefined)` on each optional field. +- **N-2. The loopback logic is correct.** WHATWG `hostname` strips userinfo, keeps the IPv6 brackets, and normalizes `0x7f000001` and `127.1`; trailing-dot and IPv4-mapped forms fail closed. The tests (live-host-guard.test.ts:8-19) miss three cases, which should be added as regression cases: `http://127.0.0.1@evil.com`, `http://127.0.0.1.nip.io` and `http://[::ffff:127.0.0.1]`. +- **N-3. live-resilience.spec.ts:36.** `tcpConnect(target.hostname)` receives `[::1]` with the brackets and fails on an IPv6 gateway. The relay also ignores https. Strip the brackets and document "http on loopback only". +- **N-4. Missing hook tests.** + - Session or agent switch during a reconcile, and during the offline wait. + - Unmount during the offline wait, to check that the `online` listener is removed. + - The code looks correct: use-chat.ts:44-53, 133, 140, 156. But nothing guards against regression. +- **N-5. use-chat.ts:50-52.** `waitForOnline` resumes only on the `online` event. Also resolve in the interval when `!isOffline()`, in case the event is missed. +- **N-6. CredentialsForm.tsx:119 (informational).** React DOM mirrors a controlled input's value into its `value` attribute, so `innerHTML` holds the secret while the person types. This happens only while the form is open, in the extension's own panel, and the attribute is cleared on submit or close. No change needed. Note it in credential-threat-model.md. + +## Tests: meaningful or tautological? + +- The hook tests are real behaviour tests: they count `streamChat`, `status` and history loads, and check the retry fields. They are not tautological. +- The gaps are B-1 to B-4, B-6 and N-4. The reducer has no `restoreTurn` test (tests/unit/chat/chat-state.test.ts). +- The fix report says 9 of 9 mutations were caught. I could not re-run them. + +Status: DONE_WITH_CONCERNS +Summary: All 8 should-fix items and the 4 claimed nits are present in the code. Nothing blocks, and there is still no automatic resend. Six state-machine gaps remain: B-1, B-2, B-4 and B-6 can lose an accepted turn or reply from view, or offer a duplicating "Send again", and B-3 and B-5 can offer "Send again" without proof the server lacks the turn. +Concerns/Blockers: Lint, typecheck, tests and build were not run because the shell is broken. B-5 needs a product decision. `E2E_DB_DSN` is not host-checked. diff --git a/plans/reports/code-reviewer-261012-0200-chat-retry-final-review.md b/plans/reports/code-reviewer-261012-0200-chat-retry-final-review.md new file mode 100644 index 0000000..bf7b799 --- /dev/null +++ b/plans/reports/code-reviewer-261012-0200-chat-retry-final-review.md @@ -0,0 +1,75 @@ +# Final review: chat send, reconcile and retry state machine + +Method: I only read the code, because Bash failed with ENAMETOOLONG. Lint, typecheck and tests were not run, and I ran no Playwright. I did not open `.env`. I read use-chat.ts, chat-state.ts, history.ts, dewee-client.ts, errors.ts, chat-events.ts, MessageList.tsx, ChatShell.tsx, both test files and the Dewee history/compaction code. + +B-1 to B-6 are in the code as the fix report describes, and each has a real behaviour test (call counts, retry fields, fake timers). None is a tautology. Invariants 1 (no auto-resend), 4 (single-owner loop, no leaks) and 6 (no secrets) hold. Invariants 2, 3 and 5 still have gaps. + +## Blocking + +**X-1. Server compaction renumbers `m`, so a delivered turn comes back with "Send again" and is shown twice (breaks 2 and 3).** +- Where: use-chat.ts:92-112 and 233-234; history.ts:43-45. +- Server evidence: ids are indices into `GetHistory` (dewee internal/http/sessions.go:228-243). Post-turn summarization calls `TruncateHistory(keepLast)` (dewee internal/agent/loop_history_sanitize.go:314). The indices restart after that. +- Scenario: a long session has baseline position 140. The send hits `stream_stalled`, but the server took the turn. The run ends and compaction keeps the last few messages, so the turn is now `m3`. `holdsTurn` sees no user turn above 140 and dispatches `restoreTurn` with `retry: true`. The history already shows the turn and the panel appends it a second time with "Send again". One click starts a duplicate run. +- The trigger correlates with long sessions, which are the ones that run long and stall. The fix report lists this risk as "not handled", and architecture.md:40 does not record it as accepted. +- Related: `fetchSessionHistory` returns `[]` after two shrink resets instead of failing (history.ts:45). The reconcile then treats an unread history as an empty one. `holdsTurn` is false, so "Send again" appears, and the 'history' dispatch also clears the transcript. +- Fix: + - Keep an anchor in `Baseline`: the role and normalized content of the message at `position`. + - If the new history lacks that anchor, or its highest position is below `baseline.position`, settle as `delivery_unconfirmed` with `retry: false`, as the `history === null` branch does. + - Make history.ts throw instead of returning `[]`. + - Test both cases. + +**X-2. A queued card continuation can be dropped with no retry (breaks 5).** +- Where: use-chat.ts:127, 260 and 414-416; ChatShell.tsx:109. +- Scenario A: two cards are answered while the reply is still streaming. `pendingContinuationRef` is a single slot, so the second continuation overwrites the first. The first card is already `resolved` on the server and is no longer in `GET /actions?status=pending`, so no card or button can ever send its `setup_action_id`. +- Scenario B: one card is answered, then the person switches session or agent, or closes the panel, before the reply ends. The scope effect clears the slot (line 260). The result is the same: the continuation is lost silently. +- Fix: queue continuations by `setup_action_id`. On a switch or unmount, send them anyway (each carries its own `session_key`; the Gateway delivers at most once), or surface each as a `retryContinuation` entry. Test both scenarios. + +## Should-fix + +**S-1. A 502, 503 or 504 from a proxy or the Cloud BFF counts as a refusal and offers "Send again" (invariant 2).** +- Where: use-chat.ts:368 and 375-394. +- Scenario: the gateway, or a BFF upstream, crashes or times out after it stored the turn, and the proxy answers 502. In a known session the panel offers an immediate retry without reconciling, and the retry creates a duplicate run. An intermediary's 5xx is not proof that the server lacks the turn. +- Fix: treat a 5xx as `mayHaveReached`. A known session reconciles with `unconfirmed`; a new chat gets `recover: 'sessions'`. Add a hook test. + +**S-2. A null baseline caused by a failed history load also uses the "any match anywhere" fallback, so a lost short turn disappears silently.** +- Where: use-chat.ts:109, 170 and 299; chat-state.ts:198 (`loadFailed` sets idle); ChatShell.tsx:290 (the composer is enabled). +- Scenario: on a flaky network the initial history load fails, so `baselineRef` stays null. The person sends "yes". The fetch is rejected while the browser is online. The reconcile then loads a history that holds an older "yes", so the turn counts as delivered and is replaced by the history. The code comment limits this fallback to "ids missing", but it covers this case too. +- Fix: when the baseline is null and a match is found, settle as `delivery_unconfirmed` with reload rather than "delivered", or keep a separate "never loaded" state. + +**S-3. Test gaps for the paths above.** There are no tests for: +- 409 `continuation_sent` counting as delivered (use-chat.ts:366); +- 5xx; +- compaction or renumbered ids; +- a queued continuation during a switch. + +## Nits + +- **N-a. The `confirmed` count drifts** (use-chat.ts:110 and 321-328). It undercounts in two cases: + - a stored turn whose stream sent `chat.error` before `chat.started`; + - a delivered turn whose reconcile could not load history (the baseline is not refreshed). + + `slice(confirmed)` also assumes no other-device turn falls between our confirmed turns. In each case only a repeated identical text is misjudged as delivered. X-1's anchor helps here as well. +- **N-b. The offline check is weak proof** (use-chat.ts:358 and 379). If Wi-Fi drops after the request bytes went out, `onLine` is false at catch time, and a new chat offers "Send again". This is the accepted heuristic. Note it in architecture.md:40 as residual risk. +- **N-c. A new send drops the unchecked turn** (use-chat.ts:297; chat-state.ts:200). It clears `uncheckedRef` and the reload button of an earlier `delivery_unconfirmed` turn. The next history load then removes that turn silently if the server lacks it. +- **N-d. The `error.delivery_unconfirmed` text is used where it does not fit** (i18n.ts:103 and vi :285). The text says "The connection dropped", but it is also shown for: + - an early stream end (use-chat.ts:345); + - a decoder fault (:371); + - a failed history read (:232). + + Suggested wording: "Delivery could not be confirmed". +- **N-e. Focus is lost after recovery actions** (MessageList.tsx:32-44). The en and vi labels exist, and the controls are native `