diff --git a/docs/system-specs/modules/learn-cron-dashboard.md b/docs/system-specs/modules/learn-cron-dashboard.md index 0ebf9e59059..0501ec36808 100644 --- a/docs/system-specs/modules/learn-cron-dashboard.md +++ b/docs/system-specs/modules/learn-cron-dashboard.md @@ -461,6 +461,7 @@ A pending tool approval has **two** pieces of state that must stay in lockstep: **MCP Discovery** (multi-provider, config-first page's Add Server modal): GET `/api/mcp/discover?q=&provider=&limit=` — concurrent fan-out over registered providers (`official` = public MCP registry at registry.modelcontextprotocol.io via `mcp_providers/official.py`, always registered; `capability` = the edition's CapabilityManager CPP seam, registered only when `available()`); a query under 2 chars short-circuits to `{results: [], providers: [...]}` without provider calls (cheap availability probe); all provider-sourced strings pass `redact_credentials` + `redact_exfiltration_urls`; results carry `installed` cross-referenced against KiroCrew scope. GET `/api/mcp/discover/detail?provider=&id=` — full description plus `install_plan` preview (the exact mcp.json spec install would write: npm→`npx -y pkg@ver`, pypi→`uvx`, oci→`docker run -i --rm`, remotes→`{url}`; priority npm>pypi>oci>remote) and `required_env` (env vars installed as `""` placeholders the user must fill). POST `/api/mcp/discover/install` `{provider, id}` — official: translates the registry entry to a spec (runtime args precede the package target for npx/uvx and are NEVER emitted for oci — publisher-controlled docker flags like `--privileged` would dissolve the container boundary, so the sandbox is pinned to `run -i --rm `) and writes through the locked `_set_kirocrew_entry` path (409 on name collision with a different spec, name gated by `_is_valid_mcp_name`); EVERY fresh official install lands `disabled` — enabling from the servers table (after reviewing the written spec and filling any env vars) is the informed-consent step, and an identical-spec reinstall never flips the user's enabled state; capability: delegates to `CapabilityManager.install_mcp` then syncs agent config; both SEL-audited as `mcp_discover_install`. The browse modal offers Install only from the detail pane, and the button stays disabled until the detail (with its install-plan preview) has loaded — pending or failed fetches never expose an active Install (list rows are status-only, and the Enter shortcut applies the same readiness predicate). **Agent Config**: GET/PUT `/api/agent/config` (read/write `~/.kiro/agents/kirocrew.json`, PUT auto-restarts sessions) **Chat**: POST `/api/chat` (SSE stream, or JSON with `?ws=1` — chunks via WebSocket), `/api/chat/slots` (CRUD, POST accepts optional `agent` field to set agent at creation; list responses include `source_links` extracted from slot messages with cached provider/number/state/CI metadata), resume from history, POST `/api/chat/slots/{slot}/generate-title`, POST `/api/chat/slots/{slot}/agent` (switch agent for slot), POST `/api/chat/slots/{slot}/fork` (fork session — copies visible messages into new slot, body: `{at_message_index?, prompt?}`, returns `{ok, key, title, messages, prompt}`, new slot has `forked_from` metadata), POST `/api/chat/slots/{slot}/edit-resend` (edit a user message and re-run; in-place truncation of `slot.messages`, body: `{index?, ts?, content}`), POST `/api/chat/slots/{slot}/rewind` (edit any past user message and re-run; fork-and-swap — truncates `slot.messages`, removes the slot's ACP session via `SessionManager.remove`, deletes orphaned kiro-cli session JSONL at `~/.kiro/sessions/cli/.json[l]`, then runs the edited prompt against a fresh ACP session under the same slot key/title/folder. Mirrors kiro-cli `/rewind`. Body: `{at_message_index?, ts?, content}`), PATCH `/api/chat/slots/{slot}/mode` (switch session mode between `""` and `"orchestrator"` — `_VALID_MODES`; 404 missing slot, 400 invalid mode, 409 while the session is running) +**Follow-up suggestions** (`suggest_followup` MCP tool → card above the composer): POST `/api/chat/slots/{slot}/followup` with `{items: [{title, description, prompt, branch?}]}` — broadcast-only (nothing persisted server-side), 404 unknown slot, re-validates `SUGGEST_FOLLOWUP_SCHEMA` server-side (≤3 items; title ≤120, description ≤600, prompt ≤8000, branch ≤80 chars full-matching `FOLLOWUP_BRANCH_RE`; hidden-Unicode stripped), redacts every field through `redact_exfiltration_urls` + `redact_credentials` — including `branch`, which is DROPPED rather than mangled when redaction alters it — and emits `followup_card` over WS. The card goes out on the OWNER-only websocket channel (`deliver_ws_owners`), never the all-clients broadcast, because an app caller can open `/api/ws` and would otherwise receive another user's complete handoff prompts. Returns `{ok, delivered}` where `delivered` is the number of owner-socket sends that actually COMPLETED — the send is awaited (`deliver_ws_owners`), not fire-and-forget, because a socket count is taken before any send runs and a window that drops in that gap would be reported as delivered; failed sends are dropped from the owner set. A card with no listener is reported to the model as not shown instead of a false success. Dashboard sessions only, at both layers: the MCP tool rejects non-`dashboard:` session keys, and BOTH endpoints require the OWNER's own identity via `is_owner_dashboard_request` — the same predicate the source-provider mutations use: an `app` claim of `""` is necessary but not sufficient, since a dashboard credential minted for a different subject carries it too and would raise cards in the owner's composer and create branches in the owner's repositories; when no owner is configured only the signed local bootstrap subjects (`local-app`, `local-startup`) are accepted, which is the standalone-local case. One carve-out: the loopback internal-secret grant sets `request["internal_auth"] = True` and NO app claim (that is the path every MCP call arrives on), so it is permitted. A request with neither marker really did skip authentication and is refused, SEL-audited. POST `/api/worktree/create` with `{repo, branch}` creates `/-wt-` on a new branch off `origin/HEAD` (falling back to `HEAD`) and returns `{ok, path, branch, base, reused}`. Security boundary: `repo` must name or sit inside a directory an existing chat slot is scoped to (the submitted path AND the resolved git toplevel are both checked, and only the server-held root is ever used as a path); sensitive paths refused; git is routed through the `sandboxed_spawn_argv` chokepoint in **strict** mode (OS isolation + scrubbed env; strict because the filter probe passes `--includes` and repo-controlled `include.path` could otherwise make git read `~/.aws/credentials` as config); a host with no sandbox backend and no `agent.sandbox_allow_unsandboxed_exec` opt-in gets a 503 instead of an unisolated spawn. On top of that: an argv list with no shell, `resource_limit_preexec`, 120s timeout, and `-c core.hooksPath= -c core.fsmonitor=false` so no repo-supplied program executes (a non-directory device has no hook to find and nowhere to plant one; an in-repo path is repo-writable and a gateway-owned temp dir is still same-uid writable between calls) — a repo declaring a `filter.*.{process,smudge,clean}` driver in EITHER repository config scope (`--local`, and `--worktree` when `extensions.worktreeConfig` is on and a `config.worktree` exists under the per-worktree `$GIT_DIR`) is refused (409) because `-c` cannot disable an arbitrary filter name; the branch must also be a ref git accepts (`foo..bar`, trailing `.`/`.lock`, and `HEAD` are rejected up front rather than after the claim); both probes pass `--includes` (git defaults it OFF for a specific-scope query, so a driver reached via `include.path` was invisible yet still ran on checkout), and an unreadable scope is refused too. The allow-list collection and the sensitive-path/`isdir` screens run on a worker thread, so a slot project on stalled storage cannot block the event loop. Concurrency: the branch is claimed atomically (`update-ref ""`) and the destination by `os.mkdir`, both before anything else, so cleanup after a failure deletes only what the request proved it created, pruning before the branch delete so an `rmtree` fallback cannot leave the claimed branch behind, and skipping the delete entirely when the post-prune listing shows another worktree holding that branch or cannot be read at all (`update-ref -d` lacks `branch -D`'s used-by-worktree guard, so deleting an adopted branch would strand that worktree on a dangling ref); a failed `worktree list` is treated as unknown (503), never as "nothing registered". Reuse requires the destination to be registered on the REQUESTED branch (`_dir_slug` keeps only a branch's last segment, so `feat/foo` and `fix/foo` collide). Both endpoints are SEL-audited. Frontend: both card actions PRE-FILL a composer and never send, appending below an unsent draft rather than replacing it (the pending-input path also persists the draft, so a plain set destroyed in-progress user text); the worktree action creates the tree, opens a session WITHOUT activating it, scopes it, and only then activates it and prefills — so the composer is never live in the default directory (and fails closed rather than prefilling an unrelated one), and deletes the session it just made if scoping fails. Per-slot card state is pruned on slot delete and in stale-slot pruning. User-facing guide: `docs/followup-suggestions.md`. **Pull-request sources**: POST `/api/source/pull-request` with `{url, refresh?}` returns the normalized full GitHub PR or GitLab MR payload. POST `/api/source/pull-request/checks` with `{url}` returns `{checks}` through a lightweight one-call GitHub or at-most-two-call GitLab path without rewriting the full-source cache. POST `/api/source/pull-request/status` with `{urls: [...]}` (bounded to `STATUS_URLS_MAX` = 64 canonicalized URLs, non-PR/MR URLs dropped, non-list bodies 400) returns `{statuses: {url: {state?, ci?}}, refreshing: [...], ttlSecs}` read straight from the same short-TTL chip-status cache the sidebar uses — it never blocks on a provider call, and schedules the bounded background refresh for stale entries, so unknown URLs are simply absent until a later poll. `refreshing` is the scheduler's own report of URLs whose value is expected to change shortly (started or already in flight; pending-cap deferrals excluded) and `ttlSecs` is the cache TTL, so a client paces its steady state by the server's TTL and re-polls within seconds of a refresh landing instead of up to one extra interval later. The Changes-tab source strip polls it to mark every PR/MR tab with its lifecycle state and CI rollup (bounded fast follow-ups, then TTL pacing; failing polls back off exponentially to a 5-minute ceiling rather than stopping, and re-poll on reconnect), and drives the selected tab from the full payload instead. The strip polls rather than riding the WS slots channel that already carries cached `state`/`ci` for sidebar chips: slot `source_links` are capped at a handful per slot while the strip shows up to 64 sources, so widening every slots push for one panel would cost every client more than one bounded request per TTL costs this one. **One truth for two surfaces**: the full-payload cache and the lightweight chip cache are kept coherent instead of expiring independently (which let the sidebar chip and the detail panel render different lifecycles for the same PR while both were nominally "fresh"). A completed full fetch projects its `state`/`draft`/`checks` onto the chip cache via `status_from_full_payload` (`record_full_payload_status`), and a chip refresh that observes a *changed* status conversely drops the full payload for that URL, so the panel's next read cannot serve a lifecycle the chip has already moved past. A structural loop-breaker guards the two comment-mirrored projections: `_refresh_check_status` records each URL's *changed* transition, and once the identical `(previous → new)` transition repeats past `_CHECK_FLAP_DAMP_THRESHOLD` consecutive refreshes — the signature of a chip↔full vocabulary divergence — it stops invalidating the full payload and emitting deltas for that URL and logs loudly once, degrading a would-be unbounded provider-polling loop to a stale glyph; a genuinely changing PR produces distinct transitions, which resets the counter and clears the damp. **Turn-boundary refresh**: when a chat slot goes idle, `DashboardState.refresh_slot_source_status` re-reads that slot's serialized chip URLs through `request_check_refresh_now`, which bypasses the chip TTL — an agent turn that opened a PR, pushed a revision, or drove a review round is the moment the remote state most likely moved, and TTL rotation alone can lag it by minutes once there are more PR-linked slots than `CHECK_STATUS_PENDING_MAX`. It is gated on at least one owner websocket (status is credential-backed and nobody else can render it, so a headless gateway spawns no provider work), scoped to the one slot that finished, floored to one forced read per URL per `_CHECK_FORCE_MIN_INTERVAL_SECS` (URLs inside the floor fall back to plain TTL pacing), and best-effort — a failure is logged and can never break turn completion. **Status deltas push instead of polling**: whenever a URL's cached `{ci, state}` changes, the owner-only `source_status` WS event carries `{url, origin, ci?, state?}` to owner sockets (`DashboardState.push_source_status`, registered once as a delta sink at app wiring and unregistered on `on_cleanup`). `origin` (`"chip"` = the lightweight path learned it, `"detail"` = a full fetch's write-through produced it) is **diagnostic only** — the client patches its cached status batch AND invalidates `['pull-request-source', url]`/`['pull-request-checks', url]` for *every* changed delta regardless of origin. It must invalidate on `"detail"` too, because that delta is emitted by the single window whose full fetch ran; only that window received the fresh HTTP payload, so the other owner windows (whose detail query is `staleTime: Infinity`) would otherwise keep rendering the pre-change lifecycle. The initiating window's resulting refetch is harmless and cannot loop: `record_full_payload_status` runs only in the *uncached* fetch path, so the refetch hits the warm 30s cache and emits no further delta. The `origin` field is retained on the wire for diagnostics and possible future requester-aware routing; no consumer branches on it today. The client additionally invalidates the mounted pull-request queries on `chat_done` for the active slot, because lifecycle/CI deltas do not cover review comments or mergeability and the detail query is otherwise `staleTime: Infinity` (it would never refetch after mount). Polling remains the safety net for a missed event. POST `/api/source/pull-request/resolve` with `{url, threadId}`, POST `/api/source/pull-request/auto-merge` with `{url, confirmImmediateMerge?}`, and POST `/api/source/pull-request/ready` with `{url}` are the three mutations. All three go through one auth/audit/error wrapper (`_owner_mutation_response`), so a client disconnect, a rejected request, and a provider failure are recorded identically across them. Every credential-backed endpoint requires the explicit dashboard-user claim `request["app"] == ""`. When `state.owner_id` is configured, all six endpoints require an exact `request["user"] == owner_id` match. When no owner is configured, only the three read endpoints accept the signed machine-local bootstrap subjects `local-app` and `local-startup`; every mutation still returns 403. App tokens, non-owners, unrelated or missing subjects, and every unconfigured-owner mutation fail closed. Every direct source request makes a best-effort SEL audit attempt with only the caller, operation, coarse outcome/reason, never URL, thread id, provider output, or credentials. SEL write failure cannot weaken a denial or replace the request's response or exception. Provider CLI calls separately emit coarse `invoked`, `completed`, `denied`, or `failed` tool-invocation events with no argv or provider-controlled text. Cancellation while reading a request or awaiting a provider is recorded as `failed/request_cancelled` when SEL is available, then the original cancellation is re-raised; because a remote mutation may already have landed, mutation cancellation is intentionally an uncertain failure outcome. Cache removal, generation advancement, and stale in-flight detachment complete before provider mutation dispatch, so a cancellation cannot preserve or repopulate pre-mutation data. A mutation invalidates **both** caches: the full-source payload and the separate short-TTL chip-status cache the sidebar and `/api/source/pull-request/status` read, which would otherwise keep serving pre-mutation `draft`/CI state for up to one TTL. The chip cache carries its own per-URL generation counter for the same reason resolve advances the full-fetch generation: a status refresh that started before the mutation captures the generation at entry and discards its result if it changed, so an in-flight fetch cannot restore superseded state. The resolve endpoint validates thread ownership and resolves a supported review thread, returning `{resolved: true}`. The auto-merge endpoint arms provider auto-merge and returns `{autoMerge: true, mergeMethod}`: on GitHub it reads the pull request node plus the repository's allowed merge methods, refuses a draft or an already-armed pull request without dispatching, picks the first repository-allowed method in squash/merge/rebase order, and calls `enablePullRequestAutoMerge`; GitLab has no separate switch, so it reads the merge request first, refuses a draft or an already-armed merge request, and otherwise issues the merge call flagged `merge_when_pipeline_succeeds`; because that call merges immediately when no pipeline is pending, the GitLab path is a merge authorization, so with no pending head pipeline the request is refused unless the body carries `confirmImmediateMerge: true`. The field must be a real JSON boolean -- coercing it would let any truthy value, notably the string `"false"`, read as consent -- and any other value is a 400. That refusal is raised as `ConfirmationRequired` and answers with `{error, confirmationRequired: true}`, which is what makes the guard live rather than a constant the client asserts: the dashboard's confirming click sends `false`, and only the server's own refusal escalates the panel to a third, explicitly-worded `Merge now` step that sends `true` and quotes the server's reason. Confirm and Cancel are separate buttons, with Cancel standing where the arming button was, so the second half of an accidental double-click backs out instead of authorizing a merge. The ready endpoint clears draft state and returns `{ready: true}`: `markPullRequestReadyForReview` on GitHub, and on GitLab the GraphQL `mergeRequestSetDraft(draft: false)` mutation, refusing when the merge request is not a draft (`draft`, or legacy `work_in_progress`). Neither ready path rewrites the title: the draft-prefix grammar stays the provider's concern, a concurrent retitle cannot be lost, and a title that merely begins with a draft-like word (`Drafting widgets`) is never mangled. GraphQL reports refusals in the body with HTTP 200, so every mutation response is inspected for transport-level `errors` and the per-mutation `errors` field and raises rather than reading as success. Both read the current draft/auto-merge state before mutating so an inapplicable action is a 400 instead of a provider error, and both invalidate the cache before dispatch on the same rule as resolve. The full payload carries `autoMerge` (GitHub `autoMergeRequest`, GitLab `merge_when_pipeline_succeeds`) so the panel renders armed auto-merge as state rather than an available action. Invalid URLs/thread IDs return 400; provider CLI, authentication, secure-spawn, audit, or direct-fetch-capacity failures return 503. Sidebar source-link extraction ignores every non-durable message role (`chunk`, `done`, `streaming`, `queued`, and `permission`) and indexes only durable message content, preventing partial output, queue placeholders, and permission prompts from scheduling credential-backed provider work. Sidebar provider refresh and cached `state`/`ci` serialization use the same read-only boundary: an exact configured-owner request, or a signed `local-app`/`local-startup` dashboard request when no owner is configured. Non-owner and app-token slot responses retain the source URL/provider/number but cannot trigger or observe credential-backed status. **Chat Folders**: GET `/api/chat/folders` (list project folders, each enriched with a computed non-persisted `history_count` — the authoritative on-disk archived-session count per folder from `ConversationLog.list_sessions()`), POST `/api/chat/folders` (create; body `{name, parent_id?, project_dir?}`, background LLM emoji-icon generation), PATCH `/api/chat/folders/{id}` (update — accepts `hidden` (bool) alongside `name`/`collapsed`/`order`/`default_agent`/`project_dir`/`icon`; moving or reviving a session into a folder auto-unhides it via `_unhide_folder`), DELETE `/api/chat/folders/{id}` (delete + ungroup its slots) Folders may be linked to a project directory (`project_dir`, validated server-side: absolute, existing, non-sensitive path): the dashboard resolves the effective directory by walking up the folder tree to the nearest ancestor with `project_dir` set (cycle-guarded), and "new chat in folder" carries the resolved directory in the slot-create flow so the session starts on the linked project. **Agents**: GET `/api/agents` (KiroCrew agent roster ordered **most-used-first** — reorders config agents + discovered project agents by `ConversationLog.agent_usage()` (turn count, then recency), falling back gracefully to config-insertion order on any failure so the dropdown never breaks or drops agents), GET `/api/agents/installed` (list all kiro-cli agents from `~/.kiro/agents/`, with `package` field extracted from filename), GET/DELETE `/api/agents/detail/{name}` (full agent config JSON; DELETE removes the config file, protected for kirocrew/kirocrew-lite) diff --git a/src/kiro_crew/dashboard/chat.py b/src/kiro_crew/dashboard/chat.py index fcf3aaaaf85..d00d100d342 100644 --- a/src/kiro_crew/dashboard/chat.py +++ b/src/kiro_crew/dashboard/chat.py @@ -52,6 +52,7 @@ api_chat_slot_create, api_chat_slot_delete, api_chat_slot_detail, + api_chat_slot_followup, api_chat_slot_interrupt, api_chat_slot_model, api_chat_slot_project, diff --git a/src/kiro_crew/dashboard/chat_handlers.py b/src/kiro_crew/dashboard/chat_handlers.py index 749d658726b..61fc0ca2518 100644 --- a/src/kiro_crew/dashboard/chat_handlers.py +++ b/src/kiro_crew/dashboard/chat_handlers.py @@ -65,7 +65,10 @@ from kiro_crew.validation import ( _AGENT_NAME_RE, ARTIFACT_SLUG_RE, + SUGGEST_FOLLOWUP_SCHEMA, + ValidationError, normalize_theme_consent_sha, + validate_tool_args, ) logger = logging.getLogger(__name__) @@ -1773,6 +1776,151 @@ async def api_chat_slot_project(request: web.Request) -> web.Response: return web.json_response({"ok": True, "project": project}) +# Fields carried per follow-up item on the wire. Kept explicit so a future +# schema addition has to be added here deliberately rather than leaking +# whatever the model happened to send into the broadcast payload. +_FOLLOWUP_TEXT_FIELDS = ("title", "description", "prompt") + + +def _redact_followup_item(item: dict) -> dict: + """Return a display-safe copy of one follow-up item. + + Every string is LLM-authored and renders in the dashboard DOM, so it goes + through the same credential + exfiltration-URL redaction as chat content + (mirrors the AskUserQuestion path in chat_runner). ``branch`` is omitted + when absent so the frontend can fall back to deriving one from the title. + """ + out: dict[str, str] = {} + for key in _FOLLOWUP_TEXT_FIELDS: + text = str(item.get(key) or "") + text, _ = redact_exfiltration_urls(text) + text, _ = redact_credentials(text) + out[key] = text + branch = item.get("branch") + if isinstance(branch, str) and branch: + # `branch` is LLM-authored too, and it travels further than the text + # fields: into a git ref, a directory name, SEL records and logs. Run the + # same redactors, and if either one CHANGES it, drop the field rather than + # ship a mangled ref — the frontend then derives a branch from the title + # (GPT review round 4). + scrubbed, _ = redact_exfiltration_urls(branch) + scrubbed, _ = redact_credentials(scrubbed) + if scrubbed == branch: + out["branch"] = branch + return out + + +def deny_non_dashboard_caller(request: web.Request, operation: str) -> web.Response | None: + """403 unless this is the dashboard OWNER's own request, else None. + + Deny-by-default, matching ``api_chat_slots_model``'s reasoning: the auth + middleware sets ``request["app"]`` on every authenticated path (``""`` for + dashboard users, the app name for app tokens), so an ABSENT key means the + middleware did not run and must refuse rather than fall through. + + An app claim of ``""`` is necessary but NOT sufficient. Both surfaces guarded + here act on owner-scoped resources — the card renders in the owner's composer + and the worktree allow-list is built from every slot's project — so identity + is checked with ``is_owner_dashboard_request``, the same predicate the source + provider mutations use: the caller must match the configured ``owner_id``, or + be a signed local bootstrap subject when no owner is configured (the + standalone-local case, where the browser's own token is minted for + ``local-app``). A dashboard token issued for a different subject would + otherwise mutate repositories it does not own (GPT review round 12). + + ONE exception, and it is the path every MCP call arrives on: a request that + presented a valid ``X-Internal-Secret`` from loopback is granted by the + middleware WITHOUT an app claim (there is no app identity to set), so it + carries ``request["internal_auth"] is True`` instead. Refusing that would + 403 ``suggest_followup`` outright — the tool could never raise a card (GPT + review, PR #461 round 9). + """ + if request.get("internal_auth") is True: + return None + # Imported here, not at module scope: source_providers imports chat state + # helpers, so a top-level import would close a cycle (same pattern as + # api_chat_slots' owner-only check-status gate above). + from kiro_crew.dashboard.handlers.source_providers import is_owner_dashboard_request + + if not is_owner_dashboard_request(request): + try: + sel().log_api_access( + caller=str(request.get("user") or "anonymous"), + operation=operation, + outcome="denied", + source="dashboard", + error="not the dashboard owner", + ) + except Exception: # pragma: no cover - audit is best-effort + logger.debug("SEL audit failed for %s denial", operation, exc_info=True) + return web.json_response({"error": "forbidden"}, status=403) + return None + + +async def api_chat_slot_followup(request: web.Request) -> web.Response: + """POST /api/chat/slots/{slot}/followup — show an agent-authored follow-up card. + + Backs the ``suggest_followup`` MCP tool. Reachable over loopback HTTP from + inside the kiro-cli process group, so the payload is re-validated here + against the same schema the MCP layer used: this endpoint is a trust + boundary in its own right, not merely a relay. + + The card is ephemeral (broadcast-only, held in frontend state) and one card + per slot: a second call replaces an unacted-on card rather than stacking. + """ + state: DashboardState = request.app["state"] + denied = deny_non_dashboard_caller(request, "chat_slot_followup") + if denied is not None: + return denied + name = request.match_info["slot"] + slot = state._slots.get(name) + if not slot: + return web.json_response({"error": "not found"}, status=404) + try: + body = await request.json() + except Exception: + return web.json_response({"error": "invalid JSON"}, status=400) + if not isinstance(body, dict): + return web.json_response({"error": "invalid JSON"}, status=400) + try: + cleaned = validate_tool_args(body, SUGGEST_FOLLOWUP_SCHEMA) + except ValidationError as exc: + return web.json_response({"error": str(exc)}, status=400) + items = [_redact_followup_item(item) for item in cleaned.get("items") or []] + if not items: + return web.json_response({"error": "items must not be empty"}, status=400) + # The card is delivered by broadcast only — nothing is stored server-side — + # so with no WS client attached the suggestions are dropped on the floor. + # Report the number of sends that COMPLETED instead of an unconditional + # success, so the MCP tool can tell the model to restate the follow-ups in + # its reply text rather than being assured they were shown and steered into + # silence (Design review, PR #461). + # + # This send is AWAITED: a socket count is taken before any send runs, so an + # owner window that disconnects in that window produced a failed send already + # reported as delivered (GPT review round 12). + # + # OWNER clients only: an app token can open /api/ws, and an all-clients + # broadcast would hand it another user's complete handoff prompts. + try: + clients = int( + await state.deliver_ws_owners( + "followup_card", + {"slot": slot.key, "items": items, "ts": time.time()}, + ) + ) + except Exception: # pragma: no cover - defensive: delivery must not 500 + logger.debug("Follow-up card delivery failed", exc_info=True) + clients = 0 + logger.info( + "Slot %s follow-up card broadcast with %d item(s) to %d client(s)", + name, + len(items), + clients, + ) + return web.json_response({"ok": True, "count": len(items), "delivered": clients}) + + _MAX_RECENT_PROJECTS = 100 diff --git a/src/kiro_crew/dashboard/handlers/worktree.py b/src/kiro_crew/dashboard/handlers/worktree.py new file mode 100644 index 00000000000..bf0a6ea1e84 --- /dev/null +++ b/src/kiro_crew/dashboard/handlers/worktree.py @@ -0,0 +1,828 @@ +"""Git worktree creation for the follow-up card's "Start in new worktree" action. + +One endpoint, ``POST /api/worktree/create``, which creates a sibling worktree of +an existing local git repository on a new branch. The follow-up card calls it +before opening a new chat session scoped to the resulting directory. + +Threat model. Both inputs are attacker-influenceable in the sense that matters +here: ``branch`` originates from an LLM (``suggest_followup``) and ``repo`` from +whatever the calling session's project happens to be. So: + +* git is invoked with an **argv list and no shell** — there is no command string + for a metacharacter to escape from — with a credential-scrubbed environment, + the POSIX resource-limit ceiling, and a wall-clock timeout. +* ``repo`` must resolve inside a directory some existing chat slot is already + scoped to (:func:`_allowed_repo_roots`). Without that barrier any + authenticated dashboard caller could name an arbitrary host directory. + Both the submitted path AND the git toplevel it resolves to are checked, so + resolving upward out of an allowed subdirectory is refused. + +Why the git spawn is sandbox-routed +----------------------------------- +:func:`_run_git` goes through the ``sandboxed_spawn_argv`` chokepoint (OS +isolation + credential-scrubbed env), matching ``git_coord.py``'s treatment of +agent-influenced git. A host with no sandbox backend and no explicit +``agent.sandbox_allow_unsandboxed_exec`` opt-in gets a 503 rather than an +unisolated spawn. + +The repo-supplied-code guards sit ON TOP of that, because isolation bounds what +a hook can reach but does not stop it running: + +* ``git worktree add`` would otherwise run the repo's ``post-checkout`` hook or + an ``core.fsmonitor`` command; both are removed by the ``-c`` overrides in + :func:`_git_no_repo_code`, which beat every config file. ``core.hooksPath`` + points at :data:`_HOOKS_SINK` (``os.devnull``) — a non-directory OS device, so + there is no ``post-checkout`` to find and no directory anyone could plant one + in. +* A ``filter..process``/``.smudge`` driver cannot be disabled generically + (driver names are arbitrary), so a repo declaring one in EITHER repository + config scope is refused instead (:func:`_checkout_filter`). + +Concurrency +----------- +The branch is claimed atomically with ``update-ref ""`` (empty old +value = "must not exist") BEFORE anything is created, so two concurrent requests +for the same branch are decided by git's ref lock and only the winner proceeds. +Cleanup removes only what a request can prove it created — the branch only if it +won the claim, the destination only if git registers it against that same branch +(or against nothing, under the per-repo lock). Same-repo requests are also +serialized in-process by :func:`_repo_lock`. + +Other input protections +----------------------- +* ``branch`` must satisfy :func:`~kiro_crew.validation.is_valid_followup_branch`, + which excludes a leading ``-`` (git would read it as a flag), ``..``, ``~``, + ``^``, ``:``, ``?``, ``*``, ``[``, ``\\`` and whitespace. +* ``repo`` is realpath'd, must be a directory, must not be a sensitive path, and + must resolve to a git work tree whose **toplevel** is used thereafter — a path + pointing anywhere inside a repo cannot be used to make git operate on a parent + it does not control. +* The destination is *derived* by this module (never supplied by the caller) and + must not already exist, so the endpoint cannot be aimed at an existing tree. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import logging +import os +import re +import shutil +import subprocess + +from aiohttp import web + +from kiro_crew.dashboard.chat_handlers import deny_non_dashboard_caller +from kiro_crew.sandbox import resource_limit_preexec, sandboxed_spawn_argv +from kiro_crew.security import is_sensitive_path +from kiro_crew.sel import sel +from kiro_crew.validation import MAX_FOLLOWUP_BRANCH, is_valid_followup_branch + +logger = logging.getLogger(__name__) + +# Wall-clock ceiling for each git invocation. `worktree add` copies a working +# tree, so it is not instant on a large repo, but it is local-only — a run +# longer than this means something is wedged (a lock, a hook prompting for +# input) and the request should fail rather than hold a connection open. +_GIT_TIMEOUT = 120 + +# Characters kept when turning a branch name into a directory suffix. +_DIR_SLUG_STRIP_RE = re.compile(r"[^A-Za-z0-9._-]+") + +# `core.hooksPath` sink. A NON-DIRECTORY, non-replaceable OS device: git finds no +# `post-checkout` under it and there is no directory anyone could drop one into. +# +# Two earlier shapes were both wrong. An in-repo sentinel +# (`.git/kirocrew-no-hooks`) is resolved relative to the repo, so whoever prepared +# the checkout could create it and put `post-checkout` inside — the suppression +# became the execution vector. A gateway-owned `mkdtemp` directory moved the path +# out of the repo but left a same-uid, process-lifetime directory that a +# compromised agent could chmod and populate between calls (GPT review, PR #461 +# rounds 5-8). `os.devnull` has no such window and needs no bookkeeping. +_HOOKS_SINK = os.devnull + + +def _git_no_repo_code() -> tuple[str, ...]: + """Config overrides that stop the REPOSITORY supplying a program to run. + + ``-c`` beats every config file, so these hold even against a hostile + ``.git/config``: + + * ``core.hooksPath`` -> :data:`_HOOKS_SINK`, so no ``post-checkout`` (the one + hook ``worktree add`` fires) can be found or planted. + * ``core.fsmonitor=false`` -> repo config can otherwise name an arbitrary + filesystem-monitor command that git spawns on index reads. + + Together these remove the repo-controlled-code-execution vector, which is + what would otherwise argue for OS-sandbox isolation on this spawn. + """ + return ("-c", f"core.hooksPath={_HOOKS_SINK}", "-c", "core.fsmonitor=false") + + +# Bound the derived directory suffix so a long branch name cannot push the +# resulting absolute path past the filesystem's component limit (255 on ext4). +_MAX_DIR_SLUG = 60 + +# Repo-local config keys that would hand git a program to run during checkout. +# `-c` cannot generically disable these (the driver name is arbitrary), so a repo +# declaring one is refused outright — see `_checkout_filter`. +_FILTER_KEY_RE = re.compile(r"^filter\.(?P.+)\.(process|smudge|clean)$", re.IGNORECASE) + +# Returned instead of a key name when a config scope could not be read at all. +# Treated as "refuse": an unreadable scope cannot be proven filter-free. +_FILTER_PROBE_FAILED = "unreadable git config" + + +_SANDBOX_REFUSAL = ( + "This host has no OS sandbox backend, so KiroCrew will not run git for you. " + "Create the worktree manually." +) + +# Prefix every failure the sandbox launcher itself reports (see `sandbox.py`'s +# `sys.exit(f"sandbox: ...")` calls). Distinguishes "isolation could not be +# established" from a genuine git error. +_SANDBOX_LAUNCHER_PREFIX = "sandbox: " + +# STRICT, not the "standard" default. `_checkout_filter` runs `git config +# --includes`, and `include.path` is repo-controlled: a hostile checkout can point +# it at `~/.aws/credentials` (or `~/.netrc`, `~/.git-credentials`) and have git +# READ that file as config. "standard" leaves those visible; "strict" bind-mounts +# them away, along with `~/.ssh` (GPT review, PR #461 round 11). Nothing here +# needs a credential: the base ref is resolved from local refs and no remote is +# contacted, so strict costs the operation nothing. +_SANDBOX_MODE = "strict" + + +class SandboxUnavailable(RuntimeError): + """No OS sandbox backend, so the git spawn is refused rather than run bare.""" + + +# One lock per repo root, so two same-repo creates in this gateway never +# interleave their check/create/cleanup sequences. Cross-request atomicity does +# not depend on this (the branch claim in `_claim_branch` is what git enforces), +# but it removes the same-destination window between the "does dest exist" probe +# and `worktree add`, which is what lets `_cleanup_partial` treat an unregistered +# leftover directory as its own. +_REPO_LOCKS: dict[str, asyncio.Lock] = {} +_MAX_REPO_LOCKS = 64 + + +def _repo_lock(root: str) -> asyncio.Lock: + """Return (creating if needed) the serialization lock for ``root``.""" + lock = _REPO_LOCKS.get(root) + if lock is None: + if len(_REPO_LOCKS) >= _MAX_REPO_LOCKS: + # Drop idle entries so a long-lived gateway that has seen many repos + # does not accumulate locks forever. Held locks are kept. + for key in [k for k, v in _REPO_LOCKS.items() if not v.locked()]: + del _REPO_LOCKS[key] + lock = _REPO_LOCKS[root] = asyncio.Lock() + return lock + + +def _run_git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + """Run git with an argv list (never a shell) inside ``cwd``, OS-sandboxed. + + Routed through the ``sandboxed_spawn_argv`` chokepoint, matching + ``git_coord.py``'s treatment of agent-influenced git: the repository is + agent-selected and the branch is LLM-authored, so this spawn takes the OS + isolation layer plus the credential-scrubbed environment rather than relying + on argument hygiene alone (GPT review, PR #461 round 9 — the earlier + ``BENIGN_SPAWNS`` classification is withdrawn). + + :exc:`SandboxUnavailable` is raised when the host has no sandbox backend and + ``agent.sandbox_allow_unsandboxed_exec`` is unset; the endpoint turns that + into a 503 telling the user to create the worktree themselves. Fail CLOSED — + a local convenience is not worth an unisolated spawn. + + The repo-supplied-code guards are still applied on top, because sandboxing + bounds what a hook could reach but does not stop it running: the ``-c`` + overrides in :func:`_git_no_repo_code` remove ``core.hooksPath`` and + ``core.fsmonitor``, and a repo declaring a checkout filter driver in either + repository config scope is refused before this runs + (:func:`_checkout_filter`). + + The remaining protections are all here: an argv list with no shell, the + POSIX resource-limit ceiling (``resource_limit_preexec`` returns ``None`` on + Windows, where ``preexec_fn`` must be ``None``), a wall-clock timeout, and + ``GIT_TERMINAL_PROMPT=0`` so a credential helper cannot block on an + interactive prompt. + """ + try: + argv, env, cleanup = sandboxed_spawn_argv( + ["git", *_git_no_repo_code(), *args], mode=_SANDBOX_MODE + ) + except RuntimeError as exc: # no sandbox backend and no explicit opt-in + raise SandboxUnavailable(str(exc)) from exc + env["GIT_TERMINAL_PROMPT"] = "0" + try: + proc = subprocess.run( + argv, + cwd=cwd, + env=env, + capture_output=True, + text=True, + timeout=_GIT_TIMEOUT, + check=False, + preexec_fn=resource_limit_preexec(), + ) + finally: + if cleanup: + with contextlib.suppress(OSError): + os.unlink(cleanup) + # The launcher can only discover SOME isolation failures in the child, after + # `wrap_argv` has already returned: `unshare(NEWNS)` is permitted by the + # backend probe but denied at exec time on hosts that restrict mount + # namespaces (GitHub Actions runners are one — errno 1/EPERM). git never + # runs, so without this the non-zero exit is misread downstream as "not a git + # repository" or "cannot list worktrees". Surface it as the same refusal a + # missing backend gets, so the user is told the truth. + if proc.returncode != 0 and (proc.stderr or "").startswith(_SANDBOX_LAUNCHER_PREFIX): + raise SandboxUnavailable((proc.stderr or "").strip()) + return proc + + +def _dir_slug(branch: str) -> str: + """Derive a filesystem-safe directory suffix from a branch name. + + Uses the last path segment ("feat/upload-limit" -> "upload-limit") so the + sibling directory does not contain a slash, and strips anything outside + ``[A-Za-z0-9._-]``. The branch has already been regex-gated by the caller; + this is about path shape, not safety. + """ + tail = branch.rstrip("/").split("/")[-1] + slug = _DIR_SLUG_STRIP_RE.sub("-", tail).strip("-.") or "followup" + return slug[:_MAX_DIR_SLUG] + + +def _resolve_base_ref(root: str) -> str: + """Pick the ref to branch from: the remote's default branch, else HEAD. + + ``origin/HEAD`` is the repo's own declaration of its default branch, which + beats hardcoding "main" (repos whose default branch is named something else, + or with a different primary remote layout, would otherwise fail). Falls back + to ``HEAD`` so a repo with no remote — or no fetched ``origin/HEAD`` — still + works, at the cost of branching from whatever is currently checked out. + """ + probe = _run_git(["rev-parse", "--verify", "--quiet", "origin/HEAD"], root) + if probe.returncode == 0 and probe.stdout.strip(): + return "origin/HEAD" + return "HEAD" + + +def _git_toplevel(repo: str) -> str | None: + """Return the work-tree root containing ``repo``, or None if not a repo.""" + probe = _run_git(["rev-parse", "--show-toplevel"], repo) + if probe.returncode != 0: + return None + top = probe.stdout.strip() + return os.path.realpath(top) if top else None + + +def _git_error(proc: subprocess.CompletedProcess[str]) -> str: + """Condense git's stderr into a single-line message for the UI.""" + text = (proc.stderr or proc.stdout or "").strip() + if not text: + return "git failed" + # Keep the first meaningful line; git prefixes most failures with "fatal:". + for line in text.splitlines(): + line = line.strip() + if line: + return line[:300] + return "git failed" + + +def _norm_path(path: str) -> str: + """Normalized, case-folded form used to compare paths against git's output.""" + return os.path.normcase(os.path.normpath(path)) + + +def _worktree_branches(root: str) -> dict[str, str] | None: + """Map ``normalized worktree path -> branch name`` for every registered tree. + + Returns ``None`` when the git query itself FAILS, which is deliberately + distinct from an empty mapping: "git could not tell us" must never be read as + "nothing is registered", because cleanup keys destructive decisions off this + answer (GPT review round 4). + + Parsed from ``worktree list --porcelain``, whose per-tree block carries a + ``branch refs/heads/`` line (absent when detached, which maps to ""). + The path alone is not enough to identify a worktree for reuse: ``_dir_slug`` + keeps only a branch's LAST segment, so ``feat/foo`` and ``fix/foo`` derive the + same destination. Matching on path only would hand back the wrong branch's + worktree as "reused". + """ + # `-z` because a worktree path may itself contain a newline: with the + # line-oriented form such a path splits across records, never matches its + # registered entry, and a retry 409s instead of reporting `reused` + # (GPT review, PR #461 round 9). In `-z` mode git NUL-terminates every + # attribute and emits an extra NUL between entries, so empty fields are + # simply skipped. + listing = _run_git(["worktree", "list", "--porcelain", "-z"], root) + if listing.returncode != 0: + return None + trees: dict[str, str] = {} + current = "" + for field in listing.stdout.split("\0"): + if not field: + continue + if field.startswith("worktree "): + current = _norm_path(field[len("worktree ") :]) + trees[current] = "" + elif field.startswith("branch ") and current: + ref = field[len("branch ") :] + trees[current] = ref[len("refs/heads/") :] if ref.startswith("refs/heads/") else ref + return trees + + +def _worktree_config_active(root: str) -> bool: + """True when this repo has a *worktree-scoped* config file git will read. + + ``extensions.worktreeConfig=true`` makes git load ``$GIT_DIR/config.worktree`` + in addition to ``.git/config``. ``$GIT_DIR`` is **per worktree**: the common + dir for the main worktree, but ``$GIT_COMMON_DIR/worktrees/`` for a linked + one — so ``--git-common-dir`` misses a linked worktree's own file entirely + (verified: a filter declared there executed during checkout while the common + dir had no ``config.worktree`` at all; PR #461 round 7). ``--absolute-git-dir`` + resolves the right directory in both cases. + + Both conditions matter: without the extension git ignores the file, and with + the extension but no file ``git config --worktree --list`` exits 128 ("unable + to read config file") — so probing unconditionally would refuse every repo + that merely enables the extension. + """ + ext = _run_git(["config", "--bool", "--get", "extensions.worktreeConfig"], root) + if ext.returncode != 0 or ext.stdout.strip() != "true": + return False + gitdir = _run_git(["rev-parse", "--absolute-git-dir"], root) + path = gitdir.stdout.strip() if gitdir.returncode == 0 else "" + if not path: + # Cannot locate GIT_DIR: assume the scope is live, so the probe below + # runs and any failure there fails closed. + return True + if not os.path.isabs(path): + path = os.path.join(root, path) + return os.path.isfile(os.path.join(path, "config.worktree")) + + +def _checkout_filter(root: str) -> str: + """Name of a repo-supplied content filter git would run on checkout, else "". + + Defense in depth for the same class the ``-c`` overrides close. A + ``.gitattributes`` entry can name a filter (``foo``) whose driver is defined + in config as ``filter.foo.process`` / ``.smudge``; ``git worktree add`` + checks files out, so that driver would run. Filter DRIVERS can only come from + a config file (never from ``.gitattributes``, and never from a remote — clone + does not transfer config), so the repository-scoped sources are the two + config scopes git reads from inside the repo: ``--local`` (``.git/config``) + and, when :func:`_worktree_config_active`, ``--worktree`` + (``$GIT_DIR/config.worktree``, per-worktree). Probing only ``--local`` was a real + hole: ``git config --local --name-only --list`` does NOT report + worktree-scoped keys, so a repo with ``extensions.worktreeConfig=true`` and + ``filter.evil.smudge`` in ``config.worktree`` passed the check and the driver + executed during checkout (verified empirically; PR #461 round 6). + + ``--includes`` is mandatory on both probes. For a *specific* scope query + (``--local``/``--worktree``) git defaults include-following OFF, so a driver + reached through ``include.path = hostile.cfg`` was invisible to the probe yet + still resolved — and executed — during checkout (verified empirically; GPT + review, PR #461 round 8). + + Rather than try to neutralize an unbounded set of ``filter..*`` keys + with ``-c``, refuse the operation and tell the user to create the worktree + themselves. A probe that fails (git error on a scope that is live) also + refuses: we cannot prove the repo is filter-free, so we do not proceed. + + Global/system config is deliberately NOT probed: that is the user's own + machine configuration (``git lfs install`` writes there), not something the + repository supplies. + """ + scopes = ["--local"] + if _worktree_config_active(root): + scopes.append("--worktree") + for scope in scopes: + proc = _run_git(["config", scope, "--includes", "--name-only", "--list"], root) + if proc.returncode != 0: + return _FILTER_PROBE_FAILED + for key in proc.stdout.splitlines(): + key = key.strip() + if _FILTER_KEY_RE.match(key): + return key[:120] + return "" + + +def _resolve_commit(root: str, ref: str) -> str: + """Commit sha for ``ref``, or "" when it does not resolve.""" + proc = _run_git(["rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], root) + return proc.stdout.strip() if proc.returncode == 0 else "" + + +def _claim_branch(root: str, branch: str, base_sha: str) -> bool: + """Atomically create ``refs/heads/`` at ``base_sha``; False if taken. + + The empty old-value argument means "the ref must not exist", so git's ref + lock decides the winner: exactly one of N concurrent requests for the same + branch gets a zero exit. That replaces the earlier check-then-create + (``_branch_head`` followed by ``worktree add -b``), where two requests could + both observe the branch as absent and the loser's cleanup would then delete + the winner's branch and working tree. + + A True return is also this request's PROOF OF CREATION: only the claimant may + later delete the branch. + """ + proc = _run_git( + ["update-ref", "--create-reflog", f"refs/heads/{branch}", base_sha, ""], + root, + ) + return proc.returncode == 0 + + +def _delete_ref_if_unchanged(root: str, branch: str, base_sha: str) -> bool: + """Delete ``refs/heads/`` only while it still points at ``base_sha``. + + ``update-ref -d `` is git's compare-and-delete: it refuses when the + ref has moved, which is what keeps cleanup from discarding commits a + concurrent process added after our claim. With no ``base_sha`` recorded (older + call sites) there is nothing to compare against, so nothing is deleted — + leaving a claimed branch behind is recoverable, deleting someone's commits is + not. + """ + if not base_sha: + logger.warning( + "worktree cleanup has no claimed sha for %s in %s; leaving the branch in place", + branch, + root, + ) + return False + proc = _run_git(["update-ref", "-d", f"refs/heads/{branch}", base_sha], root) + return proc.returncode == 0 + + +def _cleanup_partial( + root: str, + dest: str, + branch: str, + *, + claimed: bool, + created: bool, + base_sha: str = "", +) -> None: + """Best-effort unwind of a half-created worktree/branch pair. + + ``git worktree add`` can register the worktree and create the branch before + failing later in the same command (or time out mid-way), leaving artifacts + that make every retry 409 on "already exists". + + Removes ONLY what this request can PROVE it created: + + * the destination, only when ``created`` — i.e. this request's own + :func:`os.mkdir` created that directory, which is an atomic claim that fails + with ``EEXIST`` if anyone else got there first. An additional guard skips it + if git reports the path registered to a DIFFERENT branch, and if the + listing could not be read at all (``None``) the directory is still ours by + the mkdir claim, so only the deregistration is best-effort. + * the branch, only when ``claimed`` — :func:`_claim_branch` returned True, so + the ref did not exist beforehand. + + Never inferring ownership from "git lists nothing here" is the point: that + answer is also what a transient listing failure looks like, and a wrong read + would recursively delete a directory belonging to something else. + """ + if created: + registered = _worktree_branches(root) + foreign = registered is not None and registered.get(_norm_path(dest), branch) != branch + if not foreign: + _run_git(["worktree", "remove", "--force", dest], root) + if os.path.isdir(dest): + # `worktree remove` refused (e.g. never registered) — drop the + # directory we created ourselves so the retry path is clear. + shutil.rmtree(dest, ignore_errors=True) + # Prune BEFORE deleting the branch: when `worktree remove` failed and the tree + # was dropped with rmtree, git still lists the worktree as checked out on this + # branch and refuses `branch -D` ("used by worktree"). Pruning afterwards left + # the claimed branch behind, so the retry the docstring promises hit "branch + # already exists" (GPT review, PR #461 round 7). Delete is retried once after a + # second prune, and a branch that survives both is logged rather than ignored. + _run_git(["worktree", "prune"], root) + if claimed: + # A concurrent `git worktree add` (or a plain checkout) can ADOPT the + # branch we claimed while this request was failing. `update-ref -d` has + # none of `branch -D`'s "used by worktree" protection, so deleting here + # would leave that worktree sitting on a dangling ref. Re-list AFTER the + # prune — the prune is what removes our own stale registration — and keep + # the branch when any surviving worktree other than our own destination + # holds it. An unreadable listing cannot PROVE nobody adopted it, so it + # keeps the branch too: a retry reporting "already exists" is recoverable, + # breaking someone else's worktree is not (GPT review, PR #461 round 13). + registered = _worktree_branches(root) + if registered is None or any( + held == branch and path != _norm_path(dest) for path, held in registered.items() + ): + logger.warning( + "worktree cleanup left claimed branch %s in %s: another worktree " + "holds it (or the worktree list could not be read)", + branch, + root, + ) + # COMPARE-AND-DELETE, never `branch -D`: a concurrent git process can + # advance the ref between our claim and this cleanup (a commit, a push + # into it), and a force delete would leave those commits unreferenced. + # `update-ref -d ` deletes ONLY while the ref still points at + # the value we claimed, so an advanced branch is left alone (GPT review, + # PR #461 round 10). + elif _delete_ref_if_unchanged(root, branch, base_sha) is False: + _run_git(["worktree", "prune"], root) + if _delete_ref_if_unchanged(root, branch, base_sha) is False: + logger.warning( + "worktree cleanup could not delete claimed branch %s in %s; " + "a retry will report it as already existing", + branch, + root, + ) + + +def _create_worktree_sync(root: str, branch: str) -> tuple[dict, int]: + """Blocking half of the endpoint. Returns ``(json_body, http_status)``.""" + parent = os.path.dirname(root) + dest = os.path.join(parent, f"{os.path.basename(root)}-wt-{_dir_slug(branch)}") + if is_sensitive_path(dest): + return ({"error": "Access denied"}, 403) + + offending = _checkout_filter(root) + if offending: + return ( + { + "error": ( + f"This repository configures a content filter ({offending}) that git " + "would run on checkout. Create the worktree manually." + ) + }, + 409, + ) + + registered = _worktree_branches(root) + if registered is None: + return ({"error": "git could not list this repository's worktrees"}, 503) + + # Idempotent re-entry: if the destination is ALREADY the registered worktree + # for this repo ON THIS BRANCH, this is a retry of a request whose second + # half (opening the session) failed. Report success with the existing pair + # instead of 409-ing, so the card's retry can complete. Anything else at that + # path is someone else's — including a worktree for a DIFFERENT branch that + # happens to derive the same directory name — and is refused. + if os.path.exists(dest): + if registered.get(_norm_path(dest)) == branch: + return ( + {"ok": True, "path": dest, "branch": branch, "base": "", "reused": True}, + 200, + ) + return ({"error": f"Directory already exists: {dest}"}, 409) + + base = _resolve_base_ref(root) + base_sha = _resolve_commit(root, base) + if not base_sha: + return ({"error": f"Cannot resolve a commit to branch from ({base})"}, 400) + # Claim the branch BEFORE creating anything, so concurrent requests for the + # same branch are decided by git's ref lock rather than by a check that both + # can pass. `worktree add` then checks out the ref we own instead of creating + # it with `-b`. + if not _claim_branch(root, branch, base_sha): + return ({"error": f"Branch already exists: {branch}"}, 409) + + # Claim the DESTINATION the same way, with an atomic mkdir: EEXIST means + # something else owns that path, and a successful mkdir is this request's + # proof of creation — the only thing that later authorizes deleting it. + # `git worktree add` accepts an existing EMPTY directory, so pre-creating it + # costs nothing (its "already exists" refusal applies to non-empty paths). + try: + os.mkdir(dest) + except FileExistsError: + _cleanup_partial(root, dest, branch, claimed=True, created=False, base_sha=base_sha) + return ({"error": f"Directory already exists: {dest}"}, 409) + except OSError as exc: + _cleanup_partial(root, dest, branch, claimed=True, created=False, base_sha=base_sha) + return ({"error": f"Cannot create {dest}: {exc.strerror or exc}"}, 500) + + try: + proc = _run_git(["worktree", "add", dest, branch], root) + except subprocess.TimeoutExpired: + # A timeout can still leave a registered worktree behind. + _cleanup_partial(root, dest, branch, claimed=True, created=True, base_sha=base_sha) + raise + if proc.returncode != 0: + _cleanup_partial(root, dest, branch, claimed=True, created=True, base_sha=base_sha) + return ({"error": _git_error(proc)}, 400) + if not os.path.isdir(dest): + # Defensive: git reported success but the tree is not there. + _cleanup_partial(root, dest, branch, claimed=True, created=True, base_sha=base_sha) + return ({"error": "worktree add reported success but no directory was created"}, 500) + return ({"ok": True, "path": dest, "branch": branch, "base": base, "reused": False}, 200) + + +def _allowed_repo_roots(state: object) -> list[str]: + """Realpath'd project directories that some existing chat slot is scoped to. + + This is the allow-list ``repo`` must fall inside. The frontend only ever + sends the active slot's own ``project``, so constraining to this set costs + the feature nothing while removing the endpoint's arbitrary-path surface: + without it, any authenticated dashboard caller could name *any* directory on + the host and have git run against it (CodeQL: "uncontrolled data used in + path expression"). Slot projects are set through + ``/api/chat/slots/{slot}/project``, which already realpaths and + sensitive-path-screens them. + """ + roots: list[str] = [] + slots = getattr(state, "_slots", None) or {} + for slot in list(getattr(slots, "values", list)()): + project = str(getattr(slot, "project", "") or "").strip() + if not project: + continue + resolved = os.path.realpath(project) + if os.path.isdir(resolved) and resolved not in roots: + roots.append(resolved) + return roots + + +def _match_allowed_root(candidate: str, roots: list[str]) -> str | None: + """Return the allow-listed root that ``candidate`` names or sits inside. + + Returns the value FROM ``roots`` (a server-held slot project), never the + caller's string — every filesystem operation downstream then runs on a path + the server chose, which is both the point of the barrier and why CodeQL's + "uncontrolled data used in path expression" no longer applies: the request + value is used for comparison only. + + ``candidate`` must be normalized by the caller. Comparison goes through + ``os.path.normcase`` because Windows paths are case-insensitive and + ``realpath`` does not reliably canonicalize case there — without it a + differently-cased but identical path would be refused. The prefix test is + ``os.sep``-terminated, so ``/repo-evil`` does not pass as inside ``/repo``. + """ + probe = os.path.normcase(candidate) + for root in roots: + normalized = os.path.normcase(root) + if probe == normalized or probe.startswith(normalized.rstrip(os.sep) + os.sep): + return root + return None + + +async def api_worktree_create(request: web.Request) -> web.Response: + """POST ``/api/worktree/create`` with ``{repo, branch}``. + + Creates ``/-wt-`` on a new ``branch`` off the repo's + default branch. See the module docstring for the input trust model. + """ + caller = str(request.get("user") or "dashboard") + # Dashboard users only. The allow-list below is built from EVERY slot's + # project, so an app caller reaching here could create a worktree inside a + # repository belonging to another app's session (GPT review, round 8). + denied = deny_non_dashboard_caller(request, "worktree_create") + if denied is not None: + return denied + try: + body = await request.json() + except Exception: + return web.json_response({"error": "invalid JSON"}, status=400) + if not isinstance(body, dict): + return web.json_response({"error": "invalid JSON"}, status=400) + + repo = body.get("repo") + branch = body.get("branch") + if not isinstance(repo, str) or not isinstance(branch, str): + return web.json_response({"error": "repo and branch must be strings"}, status=400) + repo, branch = repo.strip(), branch.strip() + if not repo or not branch: + return web.json_response({"error": "repo and branch are required"}, status=400) + if len(branch) > MAX_FOLLOWUP_BRANCH or not is_valid_followup_branch(branch): + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"branch={branch[:120]}", + error="invalid branch name", + ) + return web.json_response({"error": "Invalid branch name"}, status=400) + + # Allow-list barrier FIRST, before the submitted value touches the + # filesystem: it is normalized and compared as a string, and what comes back + # is the server-held slot project. Every path operation from here down uses + # `repo_root` (server-chosen), never the request value. + # `_allowed_repo_roots` realpaths and stats every slot project, and the + # checks below stat again. A project on stalled network storage would block + # the event loop — and with it every session — for as long as the filesystem + # takes to answer, so all of it runs on a worker thread, per the repo's + # no-blocking-calls-on-the-loop rule (GPT review, round 8). + roots = await asyncio.to_thread(_allowed_repo_roots, request.app.get("state")) + submitted = os.path.normpath(os.path.expanduser(repo)) + repo_root = _match_allowed_root(submitted, roots) + if repo_root is None: + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"repo={submitted[:300]}", + error="outside slot project directories", + ) + return web.json_response( + { + "error": ( + "repo must be a project directory of an existing session. " + "Set the session's project first." + ) + }, + status=403, + ) + + if not await asyncio.to_thread(os.path.isdir, repo_root): + return web.json_response({"error": "repo is not a directory"}, status=400) + if await asyncio.to_thread(is_sensitive_path, repo_root): + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"repo={repo_root}", + error="sensitive path", + ) + return web.json_response({"error": "Access denied"}, status=403) + + try: + root = await asyncio.to_thread(_git_toplevel, repo_root) + except SandboxUnavailable as exc: + # Fail CLOSED: no OS isolation available, so the spawn does not happen. + logger.warning("worktree_create: sandbox unavailable: %s", exc) + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"repo={repo_root}", + error="sandbox backend unavailable", + ) + return web.json_response({"error": _SANDBOX_REFUSAL}, status=503) + except (OSError, subprocess.SubprocessError) as exc: + logger.warning("worktree_create: git toplevel probe failed: %s", exc) + return web.json_response({"error": "git is unavailable"}, status=503) + if not root: + return web.json_response({"error": "Not a git repository"}, status=400) + # Re-check the toplevel: resolving upward from an allowed subdirectory can + # land on a repo root ABOVE every allowed root, which the match above never + # saw. Without this, granting a nested directory would let git operate on an + # ancestor the caller was never granted. + if _match_allowed_root(root, roots) is None: + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"root={root}", + error="git toplevel outside slot project directories", + ) + return web.json_response( + {"error": "The repository root is outside this session's project directory."}, + status=403, + ) + if await asyncio.to_thread(is_sensitive_path, root): + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"root={root}", + error="sensitive path", + ) + return web.json_response({"error": "Access denied"}, status=403) + + try: + async with _repo_lock(root): + payload, status = await asyncio.to_thread(_create_worktree_sync, root, branch) + except subprocess.TimeoutExpired: + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="error", + resources=f"root={root} branch={branch}", + error="git timeout", + ) + return web.json_response({"error": "git timed out"}, status=504) + except SandboxUnavailable as exc: + logger.warning("worktree_create: sandbox unavailable: %s", exc) + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="denied", + resources=f"root={root} branch={branch}", + error="sandbox backend unavailable", + ) + return web.json_response({"error": _SANDBOX_REFUSAL}, status=503) + except (OSError, subprocess.SubprocessError) as exc: + logger.warning("worktree_create failed: %s", exc) + return web.json_response({"error": "worktree creation failed"}, status=500) + + sel().log_api_access( + caller=caller, + operation="worktree_create", + outcome="allowed" if status == 200 else "error", + resources=f"root={root} branch={branch} path={payload.get('path', '')}", + error="" if status == 200 else str(payload.get("error", "")), + ) + if status == 200: + logger.info("Created worktree %s (branch %s) from %s", payload.get("path"), branch, root) + return web.json_response(payload, status=status) diff --git a/src/kiro_crew/dashboard/server.py b/src/kiro_crew/dashboard/server.py index cea8b1a66f9..aa1955f2f4b 100644 --- a/src/kiro_crew/dashboard/server.py +++ b/src/kiro_crew/dashboard/server.py @@ -124,6 +124,7 @@ unregister_status_delta_sink, ) from kiro_crew.dashboard.handlers.tunnel import api_tunnel_status +from kiro_crew.dashboard.handlers.worktree import api_worktree_create from kiro_crew.dashboard.loop_watchdog import LoopStallWatchdog from kiro_crew.dashboard.origin import ( bind_address_for, @@ -1489,6 +1490,9 @@ async def _wf_nudge_authorizer( ) app.router.add_post("/api/chat/slots/{slot}/workspace", chat.api_chat_slot_workspace) app.router.add_post("/api/chat/slots/{slot}/project", chat.api_chat_slot_project) + # Follow-up suggestion card (suggest_followup MCP tool -> card below composer) + app.router.add_post("/api/chat/slots/{slot}/followup", chat.api_chat_slot_followup) + app.router.add_post("/api/worktree/create", api_worktree_create) app.router.add_get("/api/recent-projects", chat.api_recent_projects) app.router.add_patch("/api/chat/slots/{slot}/color", chat.api_chat_slot_color) # Context injection (App Kit — silent background context) diff --git a/src/kiro_crew/dashboard/state.py b/src/kiro_crew/dashboard/state.py index 88a2547f007..731cd1bebbd 100644 --- a/src/kiro_crew/dashboard/state.py +++ b/src/kiro_crew/dashboard/state.py @@ -2822,6 +2822,47 @@ def broadcast_ws(self, msg_type: str, data: object) -> None: msg = json.dumps({"type": msg_type, "data": data}) self._send_ws_all(msg) + async def deliver_ws_owners(self, msg_type: str, data: object) -> int: + """Send a typed message ONLY to owner clients; return how many sends COMPLETED. + + Use this instead of :meth:`broadcast_ws` for payloads scoped to the + dashboard user rather than to every subscriber — an app credential can + open ``/api/ws`` and lands in ``_ws_clients``, so an all-clients broadcast + of user-scoped content crosses the App Kit boundary. + + The return value is the count of sends that actually completed, for + callers whose response reports delivery. A socket count is not a delivery count: the + fire-and-forget path returns before any ``send_str`` runs, so a client + that disconnects between the count and the send yields a failed send that + was already reported as success. For an ephemeral, broadcast-only payload + (nothing is stored server-side to re-deliver) that false success is the + whole failure mode — the caller is told the user saw a card that was + dropped on the floor. + + Sends run concurrently and failures are absorbed per socket: one dead + peer must not hide a successful delivery to another window. Sockets that + are already ``closed``, and those whose send raised, are removed here — + the same cleanup the non-awaiting path performs. + """ + targets = [ws for ws in list(self._owner_ws_clients) if not ws.closed] + if not targets: + return 0 + msg = json.dumps({"type": msg_type, "data": data}) + results = await asyncio.gather( + *(ws.send_str(msg) for ws in targets), return_exceptions=True + ) + delivered = 0 + for ws, result in zip(targets, results): + if isinstance(result, BaseException): + logger.debug("Owner WS send failed (client likely disconnected): %s", result) + self._remove_ws(ws) + else: + delivered += 1 + for ws in list(self._owner_ws_clients): + if ws.closed: + self._remove_ws(ws) + return delivered + def ws_client_count(self) -> int: """Number of connected dashboard WS clients (live subscribers).""" return len(self._ws_clients) diff --git a/src/kiro_crew/dashboard/token_auth.py b/src/kiro_crew/dashboard/token_auth.py index 49a3ce1c29f..1fc30e4433c 100644 --- a/src/kiro_crew/dashboard/token_auth.py +++ b/src/kiro_crew/dashboard/token_auth.py @@ -1220,6 +1220,13 @@ async def middleware(request: web.Request, handler: object) -> web.StreamRespons resources=path, ) _log_auth(request, "internal", "granted", "") + # Mark the grant so handlers can distinguish "the internal + # loopback caller (kiro-cli / MCP) authenticated" from "no + # auth ran at all". This branch deliberately leaves + # request["app"] unset — there is no app identity — so a + # handler that fails closed on an absent app claim would + # otherwise reject every MCP call. + request["internal_auth"] = True return await handler(request) # type: ignore[operator] # Wrong secret → deny (don't fall through) _sel = _sel_fn() diff --git a/src/kiro_crew/docs/followup-suggestions.md b/src/kiro_crew/docs/followup-suggestions.md new file mode 100644 index 00000000000..1efb10883e3 --- /dev/null +++ b/src/kiro_crew/docs/followup-suggestions.md @@ -0,0 +1,185 @@ +# Follow-up Suggestions + +At the end of a turn the agent can offer concrete next steps as a card above the +chat composer. Each suggestion carries an **expanded handoff prompt** and three +actions: start it in a new git worktree, add it to the current session, or skip. + +Both non-skip actions **pre-fill a composer and stop**. Nothing is sent until +you press send, so a single click can never launch an unattended agent turn. + +## Using it + +The agent calls the `suggest_followup` MCP tool (kirocrew-core) with up to three +items: + +```json +{ + "items": [ + { + "title": "Add rate limiting to the upload endpoint", + "description": "POST /api/upload is unbounded — a single client can saturate the worker pool.", + "prompt": "In src/kiro_crew/dashboard/handlers/files.py, add a per-caller token-bucket limiter to api_file_upload ... (full standalone instruction)", + "branch": "feat/upload-rate-limit" + } + ] +} +``` + +`title` and `description` are the human-facing label. `prompt` is the payload: +it is written to be self-contained, because the agent that receives it may have +none of the originating session's context. `branch` is optional — the card +derives a `followup/` name from the title when it is absent. + +Calling the tool is the agent's own judgement call; there is no turn-boundary +hook that forces a suggestion every turn. Silence is the intended default when +there is no substantive follow-up. + +### Actions + +| Action | Effect | +| --- | --- | +| **Start in new worktree** | Creates `/-wt-` on a new branch off the repo's default branch, opens a new chat session scoped to that directory, and pre-fills its composer with the prompt. Disabled when the session has no project directory. | +| **Add to this session** | Pre-fills the current session's composer with the prompt. An unsent draft is preserved — the prompt is appended below it, not written over it. | +| **Skip** | Dismisses that one suggestion; siblings remain. The card disappears when its last item is gone. | + +## Scope and limits + +- **Dashboard only.** `suggest_followup` rejects Slack, cron, and subagent + sessions — they have no card surface. It resolves its target slot with + `_resolve_session_key_strict()`, so an unresolved identity fails closed + rather than posting a card into someone else's session. +- **Three items max**, one card **per session**. Cards are slot-keyed, so a + suggestion arriving in one session never evicts another's. A second call for + the same session replaces its unacted-on card rather than stacking. +- **Ephemeral.** The card lives in frontend state only. It survives switching + between sessions, but a full page reload drops it. Because delivery is + broadcast-only, the endpoint **awaits** the owner-socket sends and reports how + many completed; the tool tells the model to restate the follow-ups in its reply + text when that count is zero — so an unattended turn cannot silently lose the + prompts. Counting connected sockets instead would be a false success: the count + is taken before any send runs, so a window that closes in between yields a + failed send already reported as delivered. + Parking the card server-side and replaying it on reconnect is a possible + follow-up. +- **Retry-safe.** If the worktree is created but opening the session fails, the + worktree is left in place and the create endpoint recognizes its own + destination on the next attempt (`reused: true`) instead of refusing. A + `worktree add` that fails or times out part-way is unwound, so a retry is not + blocked by half-created artifacts. + +## Trust model + +Every string in an item is LLM-authored, and one of them (`branch`) reaches a +`git` invocation. Two gates apply: + +1. **MCP layer** — `SUGGEST_FOLLOWUP_SCHEMA` in `validation.py` enforces item + count, per-field types and lengths, rejects unknown fields, strips hidden + Unicode, and full-matches `branch` against `FOLLOWUP_BRANCH_RE`. That grammar + excludes a leading `-` (git would read it as a flag), `..`, `~`, `^`, `:`, + `?`, `*`, `[`, `\`, and whitespace. +2. **Gateway** — `POST /api/chat/slots/{slot}/followup` re-validates against the + same schema (the endpoint is reachable over loopback from inside the kiro-cli + process group, so it is a trust boundary, not a relay) and redacts + credentials and exfiltration URLs from every string before broadcasting. + +Both endpoints are **owner-only**. They act on owner-scoped resources — the +card renders in the owner's composer, and the worktree allow-list spans every +slot's project — so a dashboard claim alone is not enough: the caller must match +the configured owner, or be a signed local bootstrap subject when no owner is +configured (the standalone-local case, where the browser's own credential is +minted for `local-app`). App callers are refused outright. The one exception is +the loopback internal-secret path every MCP call arrives on, which is granted +with no app identity to check. + +`POST /api/worktree/create` adds its own checks: + +- `repo` must resolve **inside a directory some existing chat slot is already + scoped to**. The card only ever sends the active session's own `project`, so + this costs nothing in practice while removing the endpoint's arbitrary-path + surface. Both the submitted path and the git toplevel it resolves to are + checked, so resolving upward out of an allowed subdirectory is refused. +- git runs with an argv list and no shell, a credential-scrubbed environment, + the POSIX resource-limit ceiling, and a 120s timeout. +- **No repository-controlled code executes.** `git worktree add` would normally + run the repo's `post-checkout` hook, and repo-local config can name commands of + its own (`core.fsmonitor`). Both are suppressed with `-c` overrides, which beat + every config file. `core.hooksPath` points at `os.devnull` — a non-directory OS + device, so there is no `post-checkout` to find and nowhere to plant one. Both + earlier shapes left a writable window: an in-repo sentinel path sits in a + directory the checkout's preparer controls, and a gateway-owned temp directory + is still same-uid writable between calls. Checkout content filters are the one + such vector `-c` cannot + close — `.gitattributes` names a filter, and its `filter..process` / + `.smudge` driver comes from config under an arbitrary name — so a repo whose + **repository-scoped** config declares one is refused with a 409 telling the user + to create the worktree manually. Both scopes git reads inside a repo are probed: + `--local` (`.git/config`) and, when `extensions.worktreeConfig` is on and a + `config.worktree` file exists under the repo's **per-worktree** `$GIT_DIR`, + `--worktree` — `--local` alone does not report worktree-scoped keys, and for a + linked worktree that file lives under `/worktrees/`, not the common + dir. Both probes pass `--includes`, which git defaults OFF for a specific-scope + query: a driver reached through `include.path` would otherwise be invisible to + the probe yet still run on checkout. A scope that cannot be read at all also refuses, since an + unreadable scope cannot be proven filter-free. (Global config is not probed: that is the user's own + machine setup, e.g. `git lfs install`, not something the repository supplies; + and `git clone` never transfers config from a remote.) These guards sit on top + of OS isolation, not instead of it: the git spawn is routed through the + `sandboxed_spawn_argv` chokepoint in **strict** mode (matching `git_coord.py`'s + treatment of agent-influenced git). Strict matters because `include.path` is + repo-controlled and the filter probe passes `--includes`: a hostile checkout + could otherwise point it at `~/.aws/credentials` and have git read that file as + config. Nothing here needs a credential — the base ref comes from local refs + and no remote is contacted, and a host with no sandbox backend — and no explicit + `agent.sandbox_allow_unsandboxed_exec` opt-in — gets a **503 telling the user to + create the worktree manually** rather than an unisolated spawn. The same 503 + covers a host that passes the backend probe but denies `unshare(NEWNS)` at exec + time (GitHub Actions runners do this): the launcher reports the refusal from the + child, and that is surfaced honestly instead of being misread as "Not a git + repository". Sandboxing + bounds what a hook could reach; the `-c` overrides and the filter refusal are + what stop one running at all. +- **The branch name must be a ref git will accept.** Beyond the character + grammar, `foo..bar`, a component ending in `.` or `.lock`, and the reserved + name `HEAD` are rejected up front — git refuses them too, but only after the + branch has been claimed, which surfaced as a misleading "Branch already + exists". +- **Concurrent requests cannot destroy each other's work.** The branch is claimed + atomically before anything is created (`update-ref ""`, where the + empty old value means "must not exist"), so git's ref lock picks one winner and + the rest get a 409. Cleanup after a failed create removes only what that + request can prove it created: the branch only if it won the claim, the + destination only if git registers it against that same branch. Deletion is + compare-and-delete, and it is additionally skipped when another worktree has + since checked that branch out — `update-ref -d` has none of `branch -D`'s + "used by worktree" protection, so deleting would leave that worktree on a + dangling ref; an unreadable worktree listing keeps the branch for the same + reason, since adoption cannot be ruled out. Same-repo + requests are additionally serialized in-process. +- **Reuse is keyed on path *and* branch.** The destination slug keeps only a + branch's last segment, so `feat/foo` and `fix/foo` derive the same directory; + an existing worktree is reported as `reused` only when `worktree list + --porcelain` shows it checked out on the requested branch. Otherwise it is a + 409, never a session opened against the wrong branch. +- Sensitive paths are refused, and the destination directory is derived + server-side (never supplied by the caller) and must not already exist. + +If the new session cannot be scoped to the worktree, the frontend deletes the +session it just created rather than leaving an unscoped one behind; the worktree +survives and the create endpoint is idempotent for it, so pressing the button +again reuses it. On success the new session is explicitly activated before its +composer is pre-filled, so a session switch during creation cannot land the +prompt in an unrelated session. + +Both endpoints emit SEL audit records. + +## Files + +| Layer | Path | +| --- | --- | +| Tool declaration + dispatch | `src/kiro_crew/mcp_core.py` | +| Arg schema | `src/kiro_crew/validation.py` | +| Card endpoint | `src/kiro_crew/dashboard/chat_handlers.py` | +| Worktree endpoint | `src/kiro_crew/dashboard/handlers/worktree.py` | +| WS event → state | `website/src/hooks/useWebSocket.ts`, `website/src/store/chatSlice.ts` | +| Card UI | `website/src/components/FollowUpCard.tsx` | +| Render site | `website/src/pages/ChatPage.tsx` | diff --git a/src/kiro_crew/docs/index.md b/src/kiro_crew/docs/index.md index 10bd49c7e0a..3537dc93594 100644 --- a/src/kiro_crew/docs/index.md +++ b/src/kiro_crew/docs/index.md @@ -56,6 +56,7 @@ agent backend and Slack credentials. | Streaming STT | Live speech-to-text partials in dashboard input (Whisper local; AWS Transcribe optional) | | Memory Modes | Per-session persistent, incognito, or temporary memory | | [Feature Tips](feature-tips.md) | Occasional personalized tips above the composer pointing at features you have not used yet | +| [Follow-up Suggestions](followup-suggestions.md) | Agent-proposed next steps above the composer — start in a new git worktree, add to this session, or skip | | TUI | Terminal UI with Ink (React for CLI) — alternative to web dashboard | | [Queued-Message Editing](dashboard.md) | Edit, reorder, or cancel a chat message waiting in the queue before it runs | diff --git a/src/kiro_crew/mcp_core.py b/src/kiro_crew/mcp_core.py index 65f77168bcd..cd1e6bf718c 100644 --- a/src/kiro_crew/mcp_core.py +++ b/src/kiro_crew/mcp_core.py @@ -106,6 +106,7 @@ SKILL_SEARCH_SCHEMA, SPAWN_RUN_SCHEMA, SPAWN_SUB_AGENTS_SCHEMA, + SUGGEST_FOLLOWUP_SCHEMA, TASK_RUN_SCHEMA, WAIT_SCHEMA, WORKFLOW_AUTHOR_SCHEMA, @@ -1561,6 +1562,83 @@ def _list_tools() -> list[dict[str, Any]]: "required": ["path"], }, }, + { + "name": "suggest_followup", + "description": ( + "Offer the user up to 3 follow-up items as a card below the chat " + "composer in the CURRENT dashboard session. Each item shows a title " + "and description with three buttons: 'Start in new worktree' (creates " + "a git worktree off the project's default branch, opens a new chat " + "session scoped to it, and pre-fills the composer with your prompt), " + "'Add to this session' (pre-fills this session's composer with your " + "prompt), and 'Skip'. Both non-skip buttons PRE-FILL the composer — " + "the user still presses send — so nothing runs without their consent." + "\n\n" + "Call this at the END of a turn when you have finished the requested " + "work and see concrete next steps worth doing. Do NOT call it to ask a " + "clarifying question you need answered to continue (just ask), and do " + "not call it every turn — silence is the correct default when there is " + "no substantive follow-up." + "\n\n" + "The 'prompt' field is the real payload: write a COMPLETE, standalone " + "handoff instruction for the next agent, which may have none of this " + "session's context. Name the files, paths, constraints, and acceptance " + "criteria explicitly. 'title'/'description' are only the human-facing " + "label. Prefer 'branch' + the worktree route for work that should not " + "share this session's working tree." + "\n\n" + "Restrictions: dashboard sessions only (Slack, cron, and subagent " + "contexts are rejected — they have no card surface). One card at a " + "time per slot: a new call replaces any card the user has not yet " + "acted on." + ), + "inputSchema": { + "type": "object", + "properties": { + "items": { + "type": "array", + "maxItems": 3, + "description": "Follow-up suggestions, most valuable first.", + "items": { + "type": "object", + "properties": { + "title": { + "type": "string", + "description": ( + "Short imperative label, e.g. " + "'Add rate limiting to the upload endpoint'." + ), + }, + "description": { + "type": "string", + "description": ( + "One or two sentences on what this does and why " + "it is worth doing. Shown under the title." + ), + }, + "prompt": { + "type": "string", + "description": ( + "The expanded, self-contained instruction handed " + "to the next agent. Assume no shared context." + ), + }, + "branch": { + "type": "string", + "description": ( + "Optional git branch name for the worktree route " + "(e.g. 'feat/upload-rate-limit'). Derived from the " + "title when omitted." + ), + }, + }, + "required": ["title", "description", "prompt"], + }, + }, + }, + "required": ["items"], + }, + }, # --- Dynamic workflows (M6): author + run + monitor from chat --- { "name": "workflow_author", @@ -5249,6 +5327,64 @@ def _redact(text: str) -> str: "CWD and project-level .kiro/steering on the next message." ) + if name == "suggest_followup": + args = validate_tool_args(args, SUGGEST_FOLLOWUP_SCHEMA) + items = args.get("items") or [] + sk = _resolve_session_key_strict() + if not sk.startswith("dashboard:"): + sel().log_tool_invocation( + session_key=sk or "", + source="mcp", + tool_name="suggest_followup", + outcome="rejected", + error="non-dashboard or unresolved session", + ) + return ( + "Error: suggest_followup only works in dashboard sessions with explicit " + "identity. Slack, cron, and subagent contexts have no follow-up card " + "surface. Write the follow-ups into your reply text instead." + ) + slot_name = sk[len("dashboard:") :] + d = _post(f"/api/chat/slots/{slot_name}/followup", {"items": items}) + err_val = d.get("error") + if err_val: + sel().log_tool_invocation( + session_key=sk, + source="mcp", + tool_name="suggest_followup", + outcome="error", + error=str(err_val), + ) + return f"Error: {err_val}" + sel().log_tool_invocation( + session_key=sk, + source="mcp", + tool_name="suggest_followup", + outcome="success", + ) + count = int(d.get("count") or len(items)) + # The card is broadcast-only. With no dashboard client attached it is + # dropped, so do NOT tell the model it was shown — the handoff prompts are + # the payload of this tool, and steering the model into silence would lose + # them (Design review, PR #461). + try: + delivered = int(d.get("delivered") or 0) + except (TypeError, ValueError): + delivered = 0 + if delivered <= 0: + return ( + f"WARNING: the {count} follow-up suggestion(s) were NOT delivered — no " + "dashboard client is currently connected, and the card is not stored " + "server-side. Restate the follow-ups in your reply text now, including " + "the full prompt for each, or they are lost." + ) + return ( + f"Showed {count} follow-up suggestion(s) in this session. The user can start " + "each one in a new worktree, add it to this session, or skip it — both " + "non-skip actions pre-fill the composer, so do not assume any of them ran. " + "End your turn now instead of acting on the follow-ups yourself." + ) + def _redact_obj(obj: Any) -> Any: """Recursively redact credentials + exfiltration URLs from a response.""" if isinstance(obj, str): diff --git a/src/kiro_crew/validation.py b/src/kiro_crew/validation.py index df2efb9d67d..ba4eb6ebfa1 100644 --- a/src/kiro_crew/validation.py +++ b/src/kiro_crew/validation.py @@ -568,6 +568,131 @@ def _validate_set_project(args: dict[str, Any]) -> None: custom_validator=_validate_set_project, ) +# suggest_followup renders an agent-authored follow-up card in the calling +# dashboard slot. Every string below is LLM-authored and lands in the DOM and +# (for the worktree action) in a `git worktree add` argv, so the shapes are +# gated here at the MCP boundary rather than trusted downstream. +# +# Git branch grammar, deliberately narrower than git's own check-ref-format: +# must start alphanumeric, then alphanumerics / dot / underscore / hyphen / +# single slashes. This rejects the ref-name metacharacters that matter for the +# worktree action — leading "-" (which git would read as a flag), "..", "@{", +# "~", "^", ":", "?", "*", "[", "\", and whitespace — before the value ever +# reaches the endpoint. Anchored with \Z (not $) so a trailing newline cannot +# slip through. The endpoint re-validates; this is the first of two gates. +FOLLOWUP_BRANCH_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*(?:/[A-Za-z0-9][A-Za-z0-9._-]*)*\Z") + +# The regex above is a character grammar, so four ref shapes still slip through +# it: ``foo..bar``, a component ending in ``.``, a component ending in ``.lock``, +# and the reserved name ``HEAD``. git rejects all four, but only AFTER the branch +# has been claimed and the destination derived — the user then sees a misleading +# "Branch already exists" (GPT review, PR #461 round 9). Rejected up front +# instead, per component so ``feat/x.lock`` is caught as well as ``x.lock``. +_GIT_RESERVED_REFS = frozenset({"HEAD"}) + +# Windows reserved device names. A branch is a loose ref FILE +# (`.git/refs/heads/`), and Windows cannot create a file whose stem is +# a device name — so `feat/CON` claims fine but the checkout fails, surfacing as +# a false "Branch already exists" (GPT review, PR #461 round 10). Rejected on every +# platform so the grammar does not depend on where the gateway runs. +_WINDOWS_DEVICE_STEMS = frozenset( + {"con", "prn", "aux", "nul"} + | {f"com{n}" for n in range(1, 10)} + | {f"lpt{n}" for n in range(1, 10)} +) + + +def is_valid_followup_branch(branch: str) -> bool: + """Whether ``branch`` is a ref name git will actually accept.""" + if not branch or not FOLLOWUP_BRANCH_RE.match(branch): + return False + if ".." in branch or branch in _GIT_RESERVED_REFS: + return False + for part in branch.split("/"): + if not part or part.endswith(".") or part.endswith(".lock"): + return False + # Device names are reserved with OR without an extension (CON, CON.txt). + if part.split(".")[0].lower() in _WINDOWS_DEVICE_STEMS: + return False + return True + + +MAX_FOLLOWUP_ITEMS = 3 +MAX_FOLLOWUP_TITLE = 120 +MAX_FOLLOWUP_DESCRIPTION = 600 +# The handoff prompt is a full agent instruction, so it gets the same 8000-char +# ceiling as monitor_start's message rather than MAX_MEDIUM_STRING. +MAX_FOLLOWUP_PROMPT = 8_000 +MAX_FOLLOWUP_BRANCH = 80 + + +def _validate_followup_items(args: dict[str, Any]) -> None: + """Validate + sanitize each follow-up item dict in place. + + ``validate_field`` only sanitizes *string* list elements, so a list of + dicts arrives untouched. This walks each item, rejects unknown keys (same + fail-closed posture as ``validate_tool_args``), enforces per-field types + and lengths, and writes the sanitized values back into the dict so the + caller receives cleaned content. + """ + items = args.get("items") + if not isinstance(items, list) or not items: + raise ValidationError("items", "required (at least one follow-up item)") + allowed_keys = {"title", "description", "prompt", "branch"} + required_keys = ("title", "description", "prompt") + limits = { + "title": MAX_FOLLOWUP_TITLE, + "description": MAX_FOLLOWUP_DESCRIPTION, + "prompt": MAX_FOLLOWUP_PROMPT, + "branch": MAX_FOLLOWUP_BRANCH, + } + for idx, item in enumerate(items): + if not isinstance(item, dict): + raise ValidationError("items", f"item[{idx}]: expected object") + for key in item: + if key not in allowed_keys: + raise ValidationError("items", f"item[{idx}]: unknown field {key!r}") + for key in required_keys: + raw = item.get(key) + if not isinstance(raw, str): + raise ValidationError("items", f"item[{idx}].{key}: required string") + cleaned = sanitize_string(raw) + if not cleaned: + raise ValidationError("items", f"item[{idx}].{key}: required (empty)") + if len(cleaned) > limits[key]: + raise ValidationError( + "items", + f"item[{idx}].{key}: exceeds max length {limits[key]} " + f"(got {len(cleaned)}, trim {len(cleaned) - limits[key]} chars)", + ) + item[key] = cleaned + branch = item.get("branch") + if branch is not None: + if not isinstance(branch, str): + raise ValidationError("items", f"item[{idx}].branch: expected string") + branch = sanitize_string(branch) + if not branch: + # An explicitly-empty branch is treated as absent rather than + # an error: the frontend derives a name from the title. + item.pop("branch", None) + continue + if len(branch) > MAX_FOLLOWUP_BRANCH: + raise ValidationError( + "items", f"item[{idx}].branch: exceeds max length {MAX_FOLLOWUP_BRANCH}" + ) + if not is_valid_followup_branch(branch): + raise ValidationError("items", f"item[{idx}].branch: invalid git branch name") + item["branch"] = branch + + +SUGGEST_FOLLOWUP_SCHEMA = ToolSchema( + tool_name="suggest_followup", + fields=[ + FieldSpec("items", list, required=True, max_items=MAX_FOLLOWUP_ITEMS, item_type=dict), + ], + custom_validator=_validate_followup_items, +) + # --- Dynamic Workflows (M6) --- _WF_RUN_ID_RE = re.compile(r"^[A-Za-z0-9_\-]{1,64}$") @@ -1264,6 +1389,7 @@ def _validate_cron_add_requires_message_or_script(args: dict[str, Any]) -> None: "get_chat_session": GET_CHAT_SESSION_SCHEMA, "list_sessions": LIST_SESSIONS_SCHEMA, "set_project": SET_PROJECT_SCHEMA, + "suggest_followup": SUGGEST_FOLLOWUP_SCHEMA, "artifact_save": ARTIFACT_SAVE_SCHEMA, "artifact_get": ARTIFACT_GET_SCHEMA, "artifact_update": ARTIFACT_UPDATE_SCHEMA, diff --git a/temp-screenshots/followup-suggest/01-single-dark.png b/temp-screenshots/followup-suggest/01-single-dark.png new file mode 100644 index 00000000000..0782c84a973 Binary files /dev/null and b/temp-screenshots/followup-suggest/01-single-dark.png differ diff --git a/temp-screenshots/followup-suggest/02-single-dark-crop.png b/temp-screenshots/followup-suggest/02-single-dark-crop.png new file mode 100644 index 00000000000..c50b3bb03f3 Binary files /dev/null and b/temp-screenshots/followup-suggest/02-single-dark-crop.png differ diff --git a/temp-screenshots/followup-suggest/03-three-dark.png b/temp-screenshots/followup-suggest/03-three-dark.png new file mode 100644 index 00000000000..e1eab70848b Binary files /dev/null and b/temp-screenshots/followup-suggest/03-three-dark.png differ diff --git a/temp-screenshots/followup-suggest/04-three-dark-crop.png b/temp-screenshots/followup-suggest/04-three-dark-crop.png new file mode 100644 index 00000000000..a6526849195 Binary files /dev/null and b/temp-screenshots/followup-suggest/04-three-dark-crop.png differ diff --git a/temp-screenshots/followup-suggest/05-prefilled-composer-dark.png b/temp-screenshots/followup-suggest/05-prefilled-composer-dark.png new file mode 100644 index 00000000000..788200ef773 Binary files /dev/null and b/temp-screenshots/followup-suggest/05-prefilled-composer-dark.png differ diff --git a/temp-screenshots/followup-suggest/06-worktree-error-dark-crop.png b/temp-screenshots/followup-suggest/06-worktree-error-dark-crop.png new file mode 100644 index 00000000000..1f2924c97a2 Binary files /dev/null and b/temp-screenshots/followup-suggest/06-worktree-error-dark-crop.png differ diff --git a/temp-screenshots/followup-suggest/07-three-light.png b/temp-screenshots/followup-suggest/07-three-light.png new file mode 100644 index 00000000000..70d4f492a3a Binary files /dev/null and b/temp-screenshots/followup-suggest/07-three-light.png differ diff --git a/temp-screenshots/followup-suggest/08-three-light-crop.png b/temp-screenshots/followup-suggest/08-three-light-crop.png new file mode 100644 index 00000000000..222202fca0b Binary files /dev/null and b/temp-screenshots/followup-suggest/08-three-light-crop.png differ diff --git a/test/test_dashboard_state_ws.py b/test/test_dashboard_state_ws.py index 45157632924..7dc6ca11733 100644 --- a/test/test_dashboard_state_ws.py +++ b/test/test_dashboard_state_ws.py @@ -100,6 +100,74 @@ def test_closed_ws_removed_on_broadcast(self, state: DashboardState) -> None: assert ws_alive in state._ws_clients +class TestOwnerScopedBroadcast: + """Owner-only typed broadcast + its delivery count (PR #461).""" + + @staticmethod + def _ws(closed: bool = False) -> MagicMock: + ws = MagicMock() + ws.closed = closed + ws.send_str = AsyncMock() + return ws + + @pytest.mark.asyncio + async def test_only_owner_clients_receive_the_message(self, state: DashboardState) -> None: + owner, other = self._ws(), self._ws() + state.register_ws(owner, owner=True) + state.register_ws(other) + await state.deliver_ws_owners("followup_card", {"slot": "chat-1"}) + assert owner.send_str.await_count or owner.send_str.call_count + assert not (other.send_str.await_count or other.send_str.call_count) + + def test_count_excludes_non_owner_clients(self, state: DashboardState) -> None: + state.register_ws(self._ws()) + state.register_ws(self._ws()) + assert state.ws_client_count() == 2 + + @pytest.mark.asyncio + async def test_awaited_delivery_counts_only_completed_sends( + self, state: DashboardState + ) -> None: + """Round 12 BLOCKING: a socket count is taken BEFORE any send runs, so a + peer that drops in that window was reported as delivered. Only a send + that completed counts.""" + good, broken = self._ws(), self._ws() + broken.send_str = AsyncMock(side_effect=ConnectionResetError("peer gone")) + state.register_ws(good, owner=True) + state.register_ws(broken, owner=True) + delivered = await state.deliver_ws_owners("followup_card", {"slot": "chat-1"}) + assert delivered == 1 + assert broken not in state._owner_ws_clients + assert good in state._owner_ws_clients + + @pytest.mark.asyncio + async def test_awaited_delivery_excludes_non_owner_and_closed( + self, state: DashboardState + ) -> None: + """A closed socket receives nothing, and an app token in `_ws_clients` + must never be counted as reach for owner-scoped content.""" + state.register_ws(self._ws(), owner=True) + state.register_ws(self._ws(closed=True), owner=True) + other = self._ws() + state.register_ws(other) + assert await state.deliver_ws_owners("followup_card", {"slot": "chat-1"}) == 1 + assert not (other.send_str.await_count or other.send_str.call_count) + + @pytest.mark.asyncio + async def test_awaited_delivery_with_no_owner_clients_is_zero( + self, state: DashboardState + ) -> None: + state.register_ws(self._ws()) + assert await state.deliver_ws_owners("followup_card", {"slot": "chat-1"}) == 0 + + @pytest.mark.asyncio + async def test_no_owner_clients_is_a_noop(self, state: DashboardState) -> None: + other = self._ws() + state.register_ws(other) + assert await state.deliver_ws_owners("followup_card", {"slot": "chat-1"}) == 0 + assert not (other.send_str.await_count or other.send_str.call_count) + + class TestSlotModel: def test_model_in_to_dict(self, state: DashboardState) -> None: slot = state.get_or_create_slot("test-1", model="claude-opus-4.5") diff --git a/test/test_followup_suggest.py b/test/test_followup_suggest.py new file mode 100644 index 00000000000..802ef2b75f3 --- /dev/null +++ b/test/test_followup_suggest.py @@ -0,0 +1,453 @@ +"""Tests for the suggest_followup tool schema and POST /api/chat/slots/{slot}/followup. + +Covers the two gates the feature relies on: the MCP-layer arg schema +(:data:`SUGGEST_FOLLOWUP_SCHEMA`) and the gateway endpoint that re-validates the +same payload and broadcasts the card. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer + +from kiro_crew.dashboard.chat import api_chat_slot_followup +from kiro_crew.dashboard.state import DashboardState, _ChatSlot +from kiro_crew.validation import ( + MAX_FOLLOWUP_PROMPT, + MAX_FOLLOWUP_TITLE, + SUGGEST_FOLLOWUP_SCHEMA, + ValidationError, + validate_tool_args, +) + + +def _item(**over: object) -> dict: + base = { + "title": "Add rate limiting", + "description": "The upload endpoint is unbounded.", + "prompt": "Add a token-bucket rate limiter to POST /api/upload in server.py.", + } + base.update(over) + return base + + +def _make_app( + state: DashboardState, + *, + app_claim: str | None = "", + user: str = "owner", + internal: bool = False, +) -> web.Application: + """App whose middleware mimics ``token_auth_middleware``'s request claims. + + ``app_claim=""`` is a dashboard user, a non-empty string is an app caller, and + ``None`` leaves the key ABSENT (as if the auth middleware never ran) — which + the endpoint must treat as unauthorized rather than falling through. + """ + + @web.middleware + async def claims(request: web.Request, handler): + if app_claim is not None: + request["app"] = app_claim + if internal: + # What the auth middleware sets for a valid internal-secret request + # from loopback: granted, but with NO app identity. + request["internal_auth"] = True + request["user"] = user + return await handler(request) + + app = web.Application(middlewares=[claims]) + app["state"] = state + app.router.add_post("/api/chat/slots/{slot}/followup", api_chat_slot_followup) + return app + + +def _mock_state(slot: _ChatSlot | None = None, ws_clients: int = 1) -> DashboardState: + state = MagicMock(spec=DashboardState) + state._slots = {} + if slot: + state._slots[slot.key] = slot + # `is_owner_dashboard_request` compares the request's subject with this, so a + # MagicMock attribute here would refuse every request for the wrong reason. + state.owner_id = "owner" + state.broadcast_ws = MagicMock() + state.ws_client_count = MagicMock(return_value=ws_clients) + state.deliver_ws_owners = AsyncMock(return_value=ws_clients) + return state + + +class TestSuggestFollowupSchema: + def test_minimal_valid_item(self): + cleaned = validate_tool_args({"items": [_item()]}, SUGGEST_FOLLOWUP_SCHEMA) + assert cleaned["items"][0]["title"] == "Add rate limiting" + assert "branch" not in cleaned["items"][0] + + def test_accepts_optional_branch(self): + cleaned = validate_tool_args( + {"items": [_item(branch="feat/upload-limit")]}, SUGGEST_FOLLOWUP_SCHEMA + ) + assert cleaned["items"][0]["branch"] == "feat/upload-limit" + + def test_rejects_empty_items(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": []}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_more_than_three_items(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": [_item()] * 4}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_missing_prompt(self): + bad = _item() + del bad["prompt"] + with pytest.raises(ValidationError): + validate_tool_args({"items": [bad]}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_non_object_item(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": ["just a string"]}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_unknown_item_field(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": [_item(autoSend=True)]}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_unknown_top_level_field(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": [_item()], "slot": "x"}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_rejects_oversized_title(self): + with pytest.raises(ValidationError): + validate_tool_args( + {"items": [_item(title="x" * (MAX_FOLLOWUP_TITLE + 1))]}, SUGGEST_FOLLOWUP_SCHEMA + ) + + def test_accepts_prompt_at_limit(self): + cleaned = validate_tool_args( + {"items": [_item(prompt="x" * MAX_FOLLOWUP_PROMPT)]}, SUGGEST_FOLLOWUP_SCHEMA + ) + assert len(cleaned["items"][0]["prompt"]) == MAX_FOLLOWUP_PROMPT + + def test_rejects_whitespace_only_title(self): + with pytest.raises(ValidationError): + validate_tool_args({"items": [_item(title=" ")]}, SUGGEST_FOLLOWUP_SCHEMA) + + @pytest.mark.parametrize( + "branch", + [ + "--upload-limits", # git would read a leading dash as a flag + "feat/../../etc", # path traversal via ref name + "feat/upload limits", # whitespace + "feat/upload;rm -rf /", # shell metacharacters + "feat/upload@{0}", # reflog syntax + "feat/upload~1", + "feat/upload^", + "feat/upload:branch", + "feat//double-slash", + "/leading-slash", + ], + ) + def test_rejects_dangerous_branch_names(self, branch): + with pytest.raises(ValidationError): + validate_tool_args({"items": [_item(branch=branch)]}, SUGGEST_FOLLOWUP_SCHEMA) + + def test_empty_branch_is_treated_as_absent(self): + cleaned = validate_tool_args({"items": [_item(branch="")]}, SUGGEST_FOLLOWUP_SCHEMA) + assert "branch" not in cleaned["items"][0] + + def test_strips_hidden_unicode_from_title(self): + # Zero-width space (Cf) must not survive into the rendered card. + cleaned = validate_tool_args( + {"items": [_item(title="Add\u200brate limiting")]}, SUGGEST_FOLLOWUP_SCHEMA + ) + assert "\u200b" not in cleaned["items"][0]["title"] + + +class TestFollowupCallerIsolation: + """Round 8 HIGH: an app caller could raise a card on a slot it does not own, + and an all-clients broadcast handed it another user's handoff prompts.""" + + @pytest.mark.asyncio + async def test_non_owner_dashboard_subject_is_refused(self): + """Round 12 BLOCKING: an app claim of "" is necessary, not sufficient. + + A dashboard token minted for a different subject carries ``app == ""`` + and sailed through the round-8 gate, so it could raise cards in the + owner's composer — and, on the sibling endpoint, create worktrees in the + owner's repositories. + """ + slot = _ChatSlot("test") + state = _mock_state(slot) + app = _make_app(state, app_claim="", user="somebody-else") + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 403 + state.deliver_ws_owners.assert_not_called() + state.broadcast_ws.assert_not_called() + + @pytest.mark.asyncio + async def test_local_install_without_a_configured_owner_is_allowed(self): + """The standalone-local case: no owner_id, browser token minted for + `local-app`. Refusing it would break the feature for every install that + has not configured an owner.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + state.owner_id = "" + app = _make_app(state, app_claim="", user="local-app") + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200, await resp.text() + state.deliver_ws_owners.assert_called_once() + + @pytest.mark.asyncio + async def test_unsigned_subject_without_a_configured_owner_is_refused(self): + """No owner configured is not a free pass: only the signed local + bootstrap subjects are accepted in that mode.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + state.owner_id = "" + app = _make_app(state, app_claim="", user="drive-by") + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 403 + state.deliver_ws_owners.assert_not_called() + + @pytest.mark.asyncio + async def test_app_caller_is_refused(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + app = _make_app(state, app_claim="some-app", user="some-app") + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 403 + state.deliver_ws_owners.assert_not_called() + state.broadcast_ws.assert_not_called() + + @pytest.mark.asyncio + async def test_internal_loopback_caller_is_allowed(self): + """Round 9: the MCP path authenticates by internal secret and carries NO + app claim, so a bare deny-on-absent gate 403'd every `suggest_followup`.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + app = _make_app(state, app_claim=None, internal=True) + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200, await resp.text() + state.deliver_ws_owners.assert_called_once() + + @pytest.mark.asyncio + async def test_absent_auth_claim_is_refused(self): + """An absent claim means the auth middleware never ran — fail closed.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + app = _make_app(state, app_claim=None) + async with TestClient(TestServer(app)) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 403 + state.deliver_ws_owners.assert_not_called() + + @pytest.mark.asyncio + async def test_card_never_uses_the_all_clients_broadcast(self): + """An app caller can open /api/ws, so the card must not go to everyone.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200 + state.broadcast_ws.assert_not_called() + state.deliver_ws_owners.assert_called_once() + + @pytest.mark.asyncio + async def test_delivered_counts_owner_clients_only(self): + """`delivered` must describe the channel the card actually went down.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + state.ws_client_count = MagicMock(return_value=7) + state.deliver_ws_owners = AsyncMock(return_value=0) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200 + assert (await resp.json())["delivered"] == 0 + + +class TestRealMiddlewareIntegration: + """End-to-end through the REAL auth middleware, not a hand-rolled claims stub. + + Round 9 caught this with a stub-only suite: the loopback internal-secret branch + grants the request but sets no app claim, so a deny-on-absent gate refused every + MCP call. These tests pin the contract at the seam where it actually broke. + """ + + @staticmethod + def _app(state: DashboardState, secret: str) -> web.Application: + from kiro_crew.dashboard.token_auth import token_auth_middleware + + mw = token_auth_middleware( + mixed_internal_paths=frozenset({"/api/chat"}), + internal_secret=secret, + ) + app = web.Application(middlewares=[mw]) + app["state"] = state + app.router.add_post("/api/chat/slots/{slot}/followup", api_chat_slot_followup) + return app + + @pytest.mark.asyncio + async def test_internal_secret_from_loopback_reaches_the_handler(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(self._app(state, "s3cr3t"))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item()]}, + headers={"X-Internal-Secret": "s3cr3t"}, + ) + assert resp.status == 200, await resp.text() + state.deliver_ws_owners.assert_called_once() + + @pytest.mark.asyncio + async def test_wrong_internal_secret_never_reaches_the_handler(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(self._app(state, "s3cr3t"))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item()]}, + headers={"X-Internal-Secret": "wrong"}, + ) + assert resp.status in (401, 403) + state.deliver_ws_owners.assert_not_called() + + +class TestFollowupEndpoint: + @pytest.mark.asyncio + async def test_broadcasts_card(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200 + assert (await resp.json())["count"] == 1 + msg_type, payload = state.deliver_ws_owners.call_args[0] + assert msg_type == "followup_card" + assert payload["slot"] == "test" + assert payload["items"][0]["title"] == "Add rate limiting" + + @pytest.mark.asyncio + async def test_unknown_slot_returns_404(self): + state = _mock_state() + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/nope/followup", json={"items": [_item()]}) + assert resp.status == 404 + state.deliver_ws_owners.assert_not_called() + + @pytest.mark.asyncio + async def test_invalid_json_returns_400(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + data="not json", + headers={"Content-Type": "application/json"}, + ) + assert resp.status == 400 + state.deliver_ws_owners.assert_not_called() + + @pytest.mark.asyncio + async def test_endpoint_revalidates_schema(self): + """The endpoint is its own trust boundary, not a relay for the MCP layer.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item(branch="-rf")]}, + ) + assert resp.status == 400 + state.deliver_ws_owners.assert_not_called() + + @pytest.mark.asyncio + async def test_credentials_are_redacted_before_broadcast(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + secret = "AKIAIOSFODNN7EXAMPLE" + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item(prompt=f"Rotate the key {secret} in config.")]}, + ) + assert resp.status == 200 + payload = state.deliver_ws_owners.call_args[0][1] + assert secret not in payload["items"][0]["prompt"] + + @pytest.mark.asyncio + async def test_credential_shaped_branch_is_dropped_not_broadcast(self): + """GPT round 4 HIGH: `branch` skipped the redactors, yet it travels the + furthest — into a git ref, a directory name, SEL records and logs. A + credential-shaped value satisfies FOLLOWUP_BRANCH_RE, so the field is + dropped whenever redaction would alter it; the card then derives a branch + from the title instead. + """ + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item(branch="AKIAIOSFODNN7EXAMPLE")]}, + ) + assert resp.status == 200 + payload = state.deliver_ws_owners.call_args[0][1] + assert "branch" not in payload["items"][0] + + @pytest.mark.asyncio + async def test_ordinary_branch_survives_redaction(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post( + "/api/chat/slots/test/followup", + json={"items": [_item(branch="feat/upload-rate-limit")]}, + ) + assert resp.status == 200 + payload = state.deliver_ws_owners.call_args[0][1] + assert payload["items"][0]["branch"] == "feat/upload-rate-limit" + + @pytest.mark.asyncio + async def test_reports_connected_client_count(self): + """The tool needs the truth: a card with no listener was not shown.""" + slot = _ChatSlot("test") + state = _mock_state(slot, ws_clients=2) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert (await resp.json())["delivered"] == 2 + + @pytest.mark.asyncio + async def test_reports_zero_delivered_with_no_clients(self): + slot = _ChatSlot("test") + state = _mock_state(slot, ws_clients=0) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200 + assert (await resp.json())["delivered"] == 0 + + @pytest.mark.asyncio + async def test_client_count_failure_degrades_to_zero(self): + """A delivery error must be reported as "nobody saw it", not a 500.""" + slot = _ChatSlot("test") + state = _mock_state(slot) + state.deliver_ws_owners = AsyncMock(side_effect=RuntimeError("boom")) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json={"items": [_item()]}) + assert resp.status == 200 + assert (await resp.json())["delivered"] == 0 + state.deliver_ws_owners.assert_called_once() + + @pytest.mark.asyncio + async def test_json_array_body_rejected(self): + slot = _ChatSlot("test") + state = _mock_state(slot) + async with TestClient(TestServer(_make_app(state))) as client: + resp = await client.post("/api/chat/slots/test/followup", json=[_item()]) + assert resp.status == 400 + state.deliver_ws_owners.assert_not_called() diff --git a/test/test_mcp_followup_dispatch.py b/test/test_mcp_followup_dispatch.py new file mode 100644 index 00000000000..6df7d06afb6 --- /dev/null +++ b/test/test_mcp_followup_dispatch.py @@ -0,0 +1,93 @@ +"""Direct coverage for the ``suggest_followup`` MCP dispatch branch. + +The endpoint, the arg schema and the frontend reducers each have their own +suites, but nothing exercised the dispatch adapter in +``_call_tool_inner`` — the layer that gates on strict dashboard identity, +derives the slot path, and turns the gateway's ``delivered`` count into the +sentence the model reads (GPT review, PR #461 round 10). ``_post`` is mocked, so +the HTTP layer is out of scope here. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from kiro_crew import mcp_core + + +def _item(**over: object) -> dict: + base = { + "title": "Add rate limiting", + "description": "The upload endpoint is unbounded.", + "prompt": "Add a token-bucket limiter to POST /api/upload.", + } + base.update(over) + return base + + +class TestSuggestFollowupDispatch: + def _invoke( + self, + args: dict, + *, + session_key: str = "dashboard:test-slot", + response: dict | None = None, + ) -> str: + captured: dict = {"calls": []} + self._captured = captured + + def fake_post(path: str, body: dict | None = None) -> dict: + captured["calls"].append((path, body)) + return response if response is not None else {"ok": True, "delivered": 1, "count": 1} + + with patch.object( + mcp_core, "_resolve_session_key_strict", return_value=session_key + ), patch.object(mcp_core, "_post", side_effect=fake_post): + result = mcp_core._call_tool_inner("suggest_followup", args) + self._captured = captured + return result + + def test_posts_to_the_slot_derived_from_the_session_key(self): + result = self._invoke({"items": [_item()]}) + assert len(self._captured["calls"]) == 1 + url, body = self._captured["calls"][0] + assert url == "/api/chat/slots/test-slot/followup" + assert body is not None and body["items"][0]["title"] == "Add rate limiting" + assert "error" not in result.lower() + + @pytest.mark.parametrize( + "session_key", + ["slack:C123", "cron:nightly", "subagent:ag-1", ""], + ) + def test_non_dashboard_sessions_are_refused_without_posting(self, session_key): + """Slack/cron/subagent contexts have no card surface, and an unresolved + identity must not be allowed to guess a slot.""" + result = self._invoke({"items": [_item()]}, session_key=session_key) + assert result.startswith("Error:") + assert "dashboard sessions" in result + assert self._captured["calls"] == [] + + def test_schema_violation_is_refused_at_the_dispatch_layer(self): + """The tool re-validates before posting — the endpoint is not the only gate. + + ``_call_tool_inner`` RAISES ``ValidationError`` (the outer ``_call_tool`` + wrapper renders it for the model); what matters here is that nothing was + posted. + """ + from kiro_crew.validation import ValidationError + + with pytest.raises(ValidationError): + self._invoke({"items": [_item(branch="-rf")]}) + assert self._captured["calls"] == [] + + def test_endpoint_error_is_surfaced_to_the_model(self): + result = self._invoke({"items": [_item()]}, response={"error": "not found"}) + assert result == "Error: not found" + + def test_zero_delivered_is_reported_rather_than_a_bare_success(self): + """With no listening client the card was dropped; the model must be told so + it restates the follow-ups in its reply instead of assuming they showed.""" + result = self._invoke({"items": [_item()]}, response={"ok": True, "delivered": 0}) + assert "0" in result or "no" in result.lower() diff --git a/test/test_worktree_create.py b/test/test_worktree_create.py new file mode 100644 index 00000000000..18a45b19222 --- /dev/null +++ b/test/test_worktree_create.py @@ -0,0 +1,1030 @@ +"""Tests for POST /api/worktree/create (follow-up card "Start in new worktree"). + +The endpoint shells out to git, so the tests exercise both halves: input +rejection (branch grammar, non-repo paths, sensitive paths) and the real +happy path against a throwaway git repo in tmp_path. +""" + +from __future__ import annotations + +import functools +import os +import pathlib +import subprocess +import tempfile +from unittest.mock import MagicMock, patch + +import pytest +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer + +from kiro_crew.dashboard.handlers.worktree import ( + _FILTER_PROBE_FAILED, + SandboxUnavailable, + _checkout_filter, + _claim_branch, + _cleanup_partial, + _dir_slug, + _match_allowed_root, + _resolve_base_ref, + _resolve_commit, + _run_git, + _worktree_branches, + _worktree_config_active, + api_worktree_create, +) +from kiro_crew.validation import FOLLOWUP_BRANCH_RE, is_valid_followup_branch + + +def _branch_exists(root: str, branch: str) -> bool: + """Whether ``refs/heads/`` resolves in ``root``.""" + return bool(_resolve_commit(root, f"refs/heads/{branch}")) + + +def _make_app( + *projects: str, app_claim: str | None = "", user: str = "owner" +) -> web.Application: + """App whose state exposes one slot per allowed project directory. + + ``app_claim`` mirrors ``token_auth_middleware``'s ``request["app"]``: ``""`` + for a dashboard user, a name for an app caller, ``None`` to leave the key + absent (auth middleware never ran). + """ + + @web.middleware + async def claims(request: web.Request, handler): + if app_claim is not None: + request["app"] = app_claim + request["user"] = user + return await handler(request) + + app = web.Application(middlewares=[claims]) + state = MagicMock() + # The gate requires the OWNER's own identity, not merely a dashboard claim + # (GPT review round 12), so the mock must carry a matching owner_id — a bare + # MagicMock attribute here would 403 every request for the wrong reason. + state.owner_id = "owner" + state._slots = { + f"chat-{i}": MagicMock(project=str(p)) for i, p in enumerate(projects) if p + } + app["state"] = state + app.router.add_post("/api/worktree/create", api_worktree_create) + return app + + +def _git(*args: str, cwd) -> None: + subprocess.run( + ["git", *args], + cwd=str(cwd), + check=True, + capture_output=True, + text=True, + ) + + +@functools.lru_cache(maxsize=1) +def _sandbox_exec_reason() -> str: + """"" if a sandboxed git can actually run here, else why it cannot. + + `_run_git` routes through the OS-sandbox chokepoint, and the endpoint refuses + with a 503 when isolation cannot be established. That is a real platform + limitation, not a defect, so the tests that exercise git must SKIP there + rather than fail. A backend-availability probe is not enough: GitHub Actions + runners pass the user-namespace probe but deny `unshare(NEWNS)` at exec time + (errno 1), which the launcher can only report from the child. + """ + with tempfile.TemporaryDirectory() as tmp: + try: + proc = _run_git(["--version"], tmp) + except SandboxUnavailable as exc: + return str(exc) or "sandbox unavailable" + except OSError as exc: # pragma: no cover - no git binary at all + return f"git unavailable: {exc}" + return "" if proc.returncode == 0 else (proc.stderr or "git failed").strip() + + +def _require_sandbox_exec() -> None: + reason = _sandbox_exec_reason() + if reason: + pytest.skip(f"sandboxed git cannot run on this host: {reason[:120]}") + + +def _passthrough_spawn(argv, mode="standard", **kw): + """Stand in for ``sandboxed_spawn_argv`` so ``_run_git``'s own result handling + can be asserted on ANY host. + + The real wrapper raises when the host has no sandbox backend (Windows + runners, macOS without ``sandbox-exec``), which turns a "given this git + result…" test into a RuntimeError about isolation before the result is ever + examined. Skipping would lose the coverage, and one such test was passing on + Windows for the wrong reason — that RuntimeError is surfaced as + ``SandboxUnavailable`` too, so the assertion held without the code under test + running (GPT review round 12 / round-11 CI). + """ + return list(argv), {}, None + + +@pytest.fixture +def repo(tmp_path): + """A minimal git repo with one commit on branch `main`. + + Requests :func:`sandbox_can_exec` so every git-touching test skips (rather + than fails) on a host where isolation cannot be established — see that + fixture for why the backend probe is not sufficient. + """ + _require_sandbox_exec() + root = tmp_path / "proj" + root.mkdir() + _git("init", "-q", "-b", "main", cwd=root) + _git("config", "user.email", "test@example.com", cwd=root) + _git("config", "user.name", "Test", cwd=root) + (root / "README.md").write_text("hi\n") + _git("add", "README.md", cwd=root) + _git("commit", "-q", "-m", "init", cwd=root) + return root + + +class TestDirSlug: + def test_uses_last_path_segment(self): + assert _dir_slug("feat/upload-limit") == "upload-limit" + + def test_strips_unsafe_chars(self): + assert "/" not in _dir_slug("feat/a_b.c-d") + + def test_falls_back_when_slug_empty(self): + assert _dir_slug("feat/...") == "followup" + + def test_bounds_length(self): + assert len(_dir_slug("feat/" + "a" * 200)) <= 60 + + +class TestWorktreeCreate: + @pytest.mark.asyncio + @pytest.mark.parametrize( + "branch", + [ + "--force", + "feat/../escape", + "feat/with space", + "feat/semi;colon", + "feat/tilde~1", + "-b", + "", + ], + ) + async def test_rejects_unsafe_branch(self, repo, branch): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": branch} + ) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_rejects_non_string_inputs(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post("/api/worktree/create", json={"repo": 1, "branch": "feat/x"}) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_rejects_missing_directory(self, tmp_path): + # Reaches the git probe, so it needs a host where the sandbox can run + # (a refusal answers 503 before any directory check is reported). + _require_sandbox_exec() + async with TestClient(TestServer(_make_app(str(tmp_path)))) as client: + resp = await client.post( + "/api/worktree/create", + json={"repo": str(tmp_path / "nope"), "branch": "feat/x"}, + ) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_rejects_non_git_directory(self, tmp_path): + _require_sandbox_exec() + plain = tmp_path / "plain" + plain.mkdir() + async with TestClient(TestServer(_make_app(str(plain)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(plain), "branch": "feat/x"} + ) + assert resp.status == 400 + assert "git repository" in (await resp.json())["error"] + + @pytest.mark.asyncio + async def test_rejects_invalid_json(self): + async with TestClient(TestServer(_make_app())) as client: + resp = await client.post( + "/api/worktree/create", + data="nope", + headers={"Content-Type": "application/json"}, + ) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_creates_sibling_worktree(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", + json={"repo": str(repo), "branch": "feat/upload-limit"}, + ) + assert resp.status == 200, await resp.text() + data = await resp.json() + created = data["path"] + assert created.endswith("proj-wt-upload-limit") + # Sibling of the repo, not nested inside it. + assert created == str(repo.parent / "proj-wt-upload-limit") + assert (repo.parent / "proj-wt-upload-limit" / "README.md").is_file() + assert data["branch"] == "feat/upload-limit" + + @pytest.mark.asyncio + async def test_path_inside_repo_resolves_to_toplevel(self, repo): + """A path deeper in the tree must not make git operate on a subdirectory.""" + nested = repo / "src" / "deep" + nested.mkdir(parents=True) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(nested), "branch": "feat/deep"} + ) + assert resp.status == 200, await resp.text() + data = await resp.json() + assert data["path"] == str(repo.parent / "proj-wt-deep") + + @pytest.mark.asyncio + async def test_existing_branch_returns_409(self, repo): + _git("branch", "feat/taken", cwd=repo) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/taken"} + ) + assert resp.status == 409 + + @pytest.mark.asyncio + async def test_existing_directory_returns_409(self, repo): + (repo.parent / "proj-wt-clash").mkdir() + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/clash"} + ) + assert resp.status == 409 + + +class TestNoRepositoryCodeExecution: + """`git worktree add` must not run the repository's own post-checkout hook. + + This is the regression guard for the GPT HIGH on PR #461: the hook is + repo-controlled code, and it executing is what would otherwise demand + OS-sandbox isolation on this spawn. The control case asserts the hook WOULD + have fired without the overrides, so the test cannot silently pass because + the harness failed to install a working hook. + """ + + @staticmethod + def _install_hook(repo, marker): + hooks = repo / ".git" / "hooks" + hooks.mkdir(parents=True, exist_ok=True) + hook = hooks / "post-checkout" + hook.write_text(f'#!/bin/sh\ntouch "{marker}"\n') + hook.chmod(0o755) + + @pytest.mark.skipif(os.name != "posix", reason="shell hook script needs POSIX sh") + def test_control_hook_fires_without_overrides(self, repo, tmp_path): + marker = tmp_path / "control-marker" + self._install_hook(repo, marker) + subprocess.run( + ["git", "worktree", "add", str(tmp_path / "wt-control"), "-b", "feat/c", "HEAD"], + cwd=str(repo), + capture_output=True, + check=True, + ) + assert marker.exists(), "harness is broken: the hook never fired even unguarded" + + @pytest.mark.skipif(os.name != "posix", reason="shell hook script needs POSIX sh") + @pytest.mark.asyncio + async def test_endpoint_does_not_run_repo_hook(self, repo, tmp_path): + marker = tmp_path / "endpoint-marker" + self._install_hook(repo, marker) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/guarded"} + ) + assert resp.status == 200, await resp.text() + created = (await resp.json())["path"] + assert not marker.exists(), "repository post-checkout hook executed" + # The suppression must not break the checkout itself. + assert (pathlib.Path(created) / "README.md").is_file() + + @pytest.mark.skipif(os.name != "posix", reason="shell hook script needs POSIX sh") + @pytest.mark.asyncio + async def test_hooks_path_is_not_a_repo_writable_location(self, repo, tmp_path): + """A hook planted at the OLD in-repo sentinel path must not execute. + + Round 5 of the PR #461 review: `core.hooksPath` resolves relative to the + repository, so pointing it at `.git/kirocrew-no-hooks` left the + suppression target inside a directory the checkout's own preparer can + write. Planting `post-checkout` there turned the guard into the execution + vector. The sink is now `os.devnull`, which is not a directory at all. + """ + marker = tmp_path / "sentinel-marker" + planted = repo / ".git" / "kirocrew-no-hooks" + planted.mkdir(parents=True, exist_ok=True) + hook = planted / "post-checkout" + hook.write_text(f'#!/bin/sh\ntouch "{marker}"\n') + hook.chmod(0o755) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/sentinel"} + ) + assert resp.status == 200, await resp.text() + assert not marker.exists(), "hook planted at the in-repo sentinel path executed" + + def test_hooks_sink_is_a_non_directory_device(self): + """Round 8 HIGH: a same-uid gateway-owned directory was still plantable. + + `os.devnull` cannot be replaced or filled, so there is no window between + one git call and the next in which a hook could appear. + """ + from kiro_crew.dashboard.handlers import worktree as wt + + assert wt._HOOKS_SINK == os.devnull + assert not os.path.isdir(wt._HOOKS_SINK) + argv = wt._git_no_repo_code() + assert f"core.hooksPath={os.devnull}" in argv + assert "core.fsmonitor=false" in argv + + +class TestIdempotentReentry: + """A retry after the caller's second step failed must complete, not 409. + + The card creates the worktree, then opens a session. If the session step + fails the worktree is already on disk, so a naive retry dead-ends on both + "directory already exists" and "branch already exists" (GPT review, PR #461). + """ + + @pytest.mark.asyncio + async def test_retry_reuses_our_own_worktree(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + first = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/retry"} + ) + assert first.status == 200, await first.text() + first_body = await first.json() + assert first_body["reused"] is False + + second = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/retry"} + ) + assert second.status == 200, await second.text() + second_body = await second.json() + assert second_body["reused"] is True + assert second_body["path"] == first_body["path"] + + @pytest.mark.asyncio + async def test_unrelated_directory_at_dest_still_409s(self, repo): + """Idempotency must not adopt a directory that is not our worktree.""" + squatter = repo.parent / "proj-wt-squat" + squatter.mkdir() + (squatter / "someone-elses-file").write_text("x") + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/squat"} + ) + assert resp.status == 409 + # Untouched. + assert (squatter / "someone-elses-file").is_file() + + @pytest.mark.asyncio + async def test_failed_add_leaves_no_branch_or_directory(self, repo): + """A failing `worktree add` must not leave artifacts that block a retry. + + The branch is claimed before the add now, so the cleanup path has real + work to do: fail only the `worktree add` invocation and pass every other + git call through. + """ + real_run_git = _run_git + + def fail_add(args, cwd): + if args[:2] == ["worktree", "add"]: + return subprocess.CompletedProcess(args, 1, "", "fatal: injected failure\n") + return real_run_git(args, cwd) + + async with TestClient(TestServer(_make_app(str(repo)))) as client: + with patch( + "kiro_crew.dashboard.handlers.worktree._run_git", side_effect=fail_add + ): + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/doomed"} + ) + assert resp.status == 400 + assert not (repo.parent / "proj-wt-doomed").exists() + assert not _branch_exists(str(repo), "feat/doomed") + # And the retry path is clear: a good request now succeeds. + ok = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/doomed"} + ) + assert ok.status == 200, await ok.text() + + @pytest.mark.asyncio + async def test_unresolvable_base_creates_nothing(self, repo): + """A base ref that resolves to no commit fails before anything is made.""" + async with TestClient(TestServer(_make_app(str(repo)))) as client: + with patch( + "kiro_crew.dashboard.handlers.worktree._resolve_base_ref", + return_value="refs/heads/does-not-exist-xyz", + ): + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/nobase"} + ) + assert resp.status == 400 + assert not (repo.parent / "proj-wt-nobase").exists() + assert not _branch_exists(str(repo), "feat/nobase") + + +class TestConcurrencySafety: + """GPT review round 3, HIGH: check-then-create let two same-branch requests + both proceed, and the loser's cleanup then destroyed the winner's worktree. + """ + + def test_claim_is_atomic(self, repo): + """The second claim of the same ref must lose, not overwrite.""" + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/claim", sha) is True + assert _claim_branch(str(repo), "feat/claim", sha) is False + + @pytest.mark.asyncio + async def test_second_request_for_same_branch_409s_and_spares_the_first(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + first = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/race"} + ) + assert first.status == 200, await first.text() + path = (await first.json())["path"] + # Same branch, but the destination is gone (user moved it): the claim + # must still refuse rather than re-create and then clean up. + os.rename(path, path + "-moved") + second = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/race"} + ) + assert second.status == 409 + assert _branch_exists(str(repo), "feat/race") + assert os.path.isdir(path + "-moved") + + def test_cleanup_spares_a_worktree_registered_to_another_branch(self, repo): + """Cleanup removes only what the request can prove it created.""" + theirs = str(repo.parent / "proj-wt-theirs") + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/theirs", sha) is True + assert _run_git(["worktree", "add", theirs, "feat/theirs"], str(repo)).returncode == 0 + # A different request unwinds ITS branch, whose derived dest collides. + assert _claim_branch(str(repo), "feat/ours", sha) is True + _cleanup_partial( + str(repo), theirs, "feat/ours", claimed=True, created=True, base_sha=sha + ) + assert os.path.isdir(theirs), "another branch's worktree was destroyed" + assert _branch_exists(str(repo), "feat/theirs") + assert not _branch_exists(str(repo), "feat/ours") + + def test_cleanup_never_touches_a_directory_it_did_not_create(self, repo): + """GPT round 4 HIGH: `created=False` must mean hands off the path. + + Previously "git lists nothing here" authorized an `rmtree`, which is also + what a transient listing failure looks like. + """ + squatter = repo.parent / "proj-wt-untouched" + squatter.mkdir() + (squatter / "precious.txt").write_text("do not delete") + _cleanup_partial( + str(repo), str(squatter), "feat/whatever", claimed=False, created=False + ) + assert (squatter / "precious.txt").is_file() + + def test_cleanup_survives_a_failed_worktree_listing(self, repo): + """A listing failure must not be read as "nothing is registered".""" + ours = repo.parent / "proj-wt-ours" + ours.mkdir() + (ours / "marker").write_text("x") + with patch( + "kiro_crew.dashboard.handlers.worktree._worktree_branches", return_value=None + ): + # created=True: the mkdir claim is what authorizes removal, not the + # (unavailable) listing. + _cleanup_partial(str(repo), str(ours), "feat/ours", claimed=False, created=True) + assert not ours.exists() + + def test_cleanup_deletes_the_branch_after_an_rmtree_fallback(self, repo): + """Round 7 MEDIUM: prune must precede `branch -D`. + + When `worktree remove` fails and the directory is dropped with `rmtree`, + git still lists the worktree as checked out on that branch and refuses + `branch -D` ("used by worktree"). Pruning afterwards left the claimed + branch behind, so the retry the endpoint promises hit "branch already + exists" instead of reusing the worktree. + """ + dest = str(repo.parent / "proj-wt-stale") + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/stale", sha) is True + assert _run_git(["worktree", "add", dest, "feat/stale"], str(repo)).returncode == 0 + real_run_git = _run_git + + def _fail_worktree_remove(args, cwd): + if args[:2] == ["worktree", "remove"]: + return subprocess.CompletedProcess(args, 1, "", "fatal: forced failure") + return real_run_git(args, cwd) + + with patch( + "kiro_crew.dashboard.handlers.worktree._run_git", side_effect=_fail_worktree_remove + ): + _cleanup_partial( + str(repo), dest, "feat/stale", claimed=True, created=True, base_sha=sha + ) + assert not os.path.isdir(dest) + assert not _branch_exists(str(repo), "feat/stale"), "claimed branch survived cleanup" + + def test_cleanup_spares_a_branch_another_worktree_adopted(self, repo): + """Round 13 BLOCKING: `update-ref -d` has no "used by worktree" guard. + + A concurrent `git worktree add` can check out the branch this request + claimed while the request is failing. Compare-and-delete still matched + (the ref had not moved), so cleanup deleted it out from under the other + worktree, leaving it on a dangling ref. + """ + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/adopted", sha) is True + # Somebody else checks the claimed branch out before our cleanup runs. + theirs = str(repo.parent / "proj-wt-adopted") + assert _run_git(["worktree", "add", theirs, "feat/adopted"], str(repo)).returncode == 0 + ours = str(repo.parent / "proj-wt-ours-failed") + _cleanup_partial( + str(repo), ours, "feat/adopted", claimed=True, created=False, base_sha=sha + ) + assert _branch_exists(str(repo), "feat/adopted"), "deleted a branch in use" + head = _run_git(["rev-parse", "--abbrev-ref", "HEAD"], theirs) + assert head.stdout.strip() == "feat/adopted" + + def test_cleanup_keeps_the_branch_when_the_listing_is_unreadable(self, repo): + """Adoption cannot be ruled out from an unreadable listing, and a retry + reporting "already exists" is recoverable where a broken worktree is not.""" + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/unknown", sha) is True + with patch( + "kiro_crew.dashboard.handlers.worktree._worktree_branches", return_value=None + ): + _cleanup_partial( + str(repo), + str(repo.parent / "proj-wt-unknown"), + "feat/unknown", + claimed=True, + created=False, + base_sha=sha, + ) + assert _branch_exists(str(repo), "feat/unknown") + + def test_cleanup_spares_a_branch_that_advanced_after_the_claim(self, repo, tmp_path): + """Round 10 HIGH: `branch -D` force-deletes, so a concurrent commit landing + on the claimed ref between the claim and the cleanup was discarded with it. + + Compare-and-delete (`update-ref -d `) refuses once the ref has + moved, so those commits stay reachable. + """ + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/advanced", sha) is True + # A concurrent process advances the claimed ref (a commit pushed into it). + wt = tmp_path / "concurrent" + assert _run_git( + ["worktree", "add", str(wt), "feat/advanced"], str(repo) + ).returncode == 0 + (wt / "new.txt").write_text("work someone else did\n") + _git("add", "new.txt", cwd=wt) + _git("-c", "user.email=a@b.c", "-c", "user.name=a", "commit", "-qm", "concurrent", cwd=wt) + advanced = _resolve_commit(str(repo), "refs/heads/feat/advanced") + assert advanced and advanced != sha + # Our create fails and unwinds, still believing the ref is at `sha`. + _cleanup_partial( + str(repo), + str(repo.parent / "proj-wt-advanced"), + "feat/advanced", + claimed=True, + created=False, + base_sha=sha, + ) + assert _resolve_commit(str(repo), "refs/heads/feat/advanced") == advanced, ( + "cleanup discarded commits added after the claim" + ) + + def test_cleanup_leaves_a_branch_it_did_not_claim(self, repo): + sha = _resolve_commit(str(repo), "HEAD") + assert _claim_branch(str(repo), "feat/preexisting", sha) is True + _cleanup_partial( + str(repo), + str(repo.parent / "proj-wt-preexisting"), + "feat/preexisting", + claimed=False, + created=False, + ) + assert _branch_exists(str(repo), "feat/preexisting") + + @pytest.mark.asyncio + async def test_unreadable_worktree_list_is_a_503_not_a_create(self, repo): + """If git cannot enumerate worktrees, refuse rather than guess.""" + async with TestClient(TestServer(_make_app(str(repo)))) as client: + with patch( + "kiro_crew.dashboard.handlers.worktree._worktree_branches", return_value=None + ): + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/blind"} + ) + assert resp.status == 503, await resp.text() + assert not (repo.parent / "proj-wt-blind").exists() + assert not _branch_exists(str(repo), "feat/blind") + + +class TestSlugCollision: + """GPT review round 3, MEDIUM: `_dir_slug` keeps only a branch's last + segment, so `feat/foo` and `fix/foo` derive the same destination. Reuse keyed + on the path alone handed back the wrong branch's worktree. + """ + + @pytest.mark.asyncio + async def test_same_slug_different_branch_is_not_reused(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + first = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/shared"} + ) + assert first.status == 200, await first.text() + dest = (await first.json())["path"] + second = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "fix/shared"} + ) + assert second.status == 409, await second.text() + body = await second.json() + assert "already exists" in body["error"] + # The first worktree is untouched and still on its own branch. + assert _worktree_branches(str(repo))[os.path.normcase(dest)] == "feat/shared" + assert not _branch_exists(str(repo), "fix/shared") + + +class TestCheckoutFilters: + """GPT review round 3, HIGH: `.gitattributes` can name a content filter whose + driver is defined in repo-local config, and checkout runs it. `-c` cannot + disable an arbitrary filter name, so such a repo is refused. + """ + + @pytest.mark.asyncio + @pytest.mark.parametrize("key", ["filter.evil.process", "filter.evil.smudge"]) + async def test_local_filter_config_is_refused(self, repo, key): + _git("config", "--local", key, "sh -c 'touch /tmp/pwned'", cwd=repo) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/filtered"} + ) + assert resp.status == 409, await resp.text() + body = await resp.json() + assert "content filter" in body["error"] + assert not (repo.parent / "proj-wt-filtered").exists() + assert not _branch_exists(str(repo), "feat/filtered") + + @pytest.mark.asyncio + async def test_unrelated_local_config_is_not_refused(self, repo): + """Only filter drivers gate the operation, not config in general.""" + _git("config", "--local", "filter.evil.required", "true", cwd=repo) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/allowed"} + ) + assert resp.status == 200, await resp.text() + + @pytest.mark.asyncio + @pytest.mark.parametrize("key", ["filter.evil.process", "filter.evil.smudge"]) + async def test_worktree_scoped_filter_config_is_refused(self, repo, key): + """Round 6 HIGH: `--local` does not report worktree-scoped keys. + + With `extensions.worktreeConfig=true` git also reads + `$GIT_COMMON_DIR/config.worktree`. A filter driver declared only there was + invisible to the old `--local`-only probe, and `git worktree add` executed + it during checkout (verified empirically before this fix). + """ + _git("config", "extensions.worktreeConfig", "true", cwd=repo) + _git("config", "--worktree", key, "sh -c 'touch /tmp/pwned'", cwd=repo) + # Precondition: the old probe genuinely could not see this key. + local = _run_git(["config", "--local", "--name-only", "--list"], str(repo)) + assert key not in local.stdout.splitlines() + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/wtfiltered"} + ) + assert resp.status == 409, await resp.text() + body = await resp.json() + assert "content filter" in body["error"] + assert not (repo.parent / "proj-wt-wtfiltered").exists() + assert not _branch_exists(str(repo), "feat/wtfiltered") + + @pytest.mark.asyncio + async def test_linked_worktree_scoped_filter_config_is_refused(self, repo, tmp_path): + """Round 7 HIGH: for a LINKED worktree, `config.worktree` lives under + `$GIT_DIR` (`/worktrees/`), not under the common dir. + + Probing the common dir therefore missed a filter declared in a linked + worktree's own config — `_worktree_config_active` returned False, the + `--worktree` scope was skipped, and the driver executed during checkout + (verified empirically before this fix). + """ + _git("config", "extensions.worktreeConfig", "true", cwd=repo) + linked = tmp_path / "linked" + _git("worktree", "add", str(linked), "-b", "linked-br", "HEAD", cwd=repo) + _git("config", "--worktree", "filter.evil.smudge", "sh -c 'touch /tmp/pwned'", cwd=linked) + # Precondition: the file is NOT where the common-dir probe looked. + common = _run_git(["rev-parse", "--git-common-dir"], str(linked)).stdout.strip() + gitdir = _run_git(["rev-parse", "--absolute-git-dir"], str(linked)).stdout.strip() + assert not os.path.isfile(os.path.join(common, "config.worktree")) + assert os.path.isfile(os.path.join(gitdir, "config.worktree")) + assert _worktree_config_active(str(linked)) + async with TestClient(TestServer(_make_app(str(linked)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(linked), "branch": "feat/linked"} + ) + assert resp.status == 409, await resp.text() + assert "content filter" in (await resp.json())["error"] + assert not _branch_exists(str(repo), "feat/linked") + + @pytest.mark.asyncio + async def test_worktree_config_enabled_but_empty_still_succeeds(self, repo): + """The extension alone must not refuse: `--worktree --list` exits 128 when + no `config.worktree` file exists, and that is not a filter.""" + _git("config", "extensions.worktreeConfig", "true", cwd=repo) + assert not _worktree_config_active(str(repo)) + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/extonly"} + ) + assert resp.status == 200, await resp.text() + + def test_probe_failure_fails_closed(self, repo): + """An unreadable config scope refuses rather than assuming "no filter".""" + failed = subprocess.CompletedProcess(args=["git"], returncode=128, stdout="", stderr="x") + with patch( + "kiro_crew.dashboard.handlers.worktree._run_git", return_value=failed + ): + assert _checkout_filter(str(repo)) == _FILTER_PROBE_FAILED + + @pytest.mark.asyncio + async def test_included_filter_config_is_refused(self, repo, tmp_path): + """Round 8 HIGH: `include.path` hid the driver from the probe. + + For a SPECIFIC scope query (`--local`/`--worktree`) git defaults + include-following OFF, so a `filter.*.smudge` reached through + `include.path` was invisible to the probe while still resolving — and + executing — during checkout (verified empirically before this fix). + """ + included = tmp_path / "inc.cfg" + included.write_text('[filter "evil"]\n\tsmudge = "sh -c \\"touch /tmp/pwned\\""\n') + _git("config", "--local", "include.path", str(included), cwd=repo) + # Preconditions: resolvable by git, invisible without --includes. + resolved = _run_git(["config", "--includes", "--get", "filter.evil.smudge"], str(repo)) + assert resolved.stdout.strip() + blind = _run_git(["config", "--local", "--name-only", "--list"], str(repo)) + assert not [k for k in blind.stdout.splitlines() if k.startswith("filter.")] + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/included"} + ) + assert resp.status == 409, await resp.text() + assert "content filter" in (await resp.json())["error"] + assert not _branch_exists(str(repo), "feat/included") + + +class TestRound9Hardening: + """Regressions for the round-9 review findings.""" + + @pytest.mark.parametrize( + "bad", + [ + "foo..bar", + "foo.", + "foo.lock", + "HEAD", + "feat/x.lock", + # Windows reserved device stems: a loose ref is a FILE, and these + # cannot be created on Windows (with or without an extension). + "CON", + "con", + "feat/AUX", + "NUL", + "COM1", + "lpt9", + "feat/con.txt", + ], + ) + def test_git_invalid_refs_are_rejected_up_front(self, bad): + """The character grammar accepted refs git itself refuses, and the failure + then surfaced as a misleading "Branch already exists".""" + assert FOLLOWUP_BRANCH_RE.match(bad), "precondition: the regex alone allows it" + assert not is_valid_followup_branch(bad) + + @pytest.mark.parametrize("good", ["feat/upload-limit", "followup/add-rate-limits", "x.y"]) + def test_ordinary_branches_still_pass(self, good): + assert is_valid_followup_branch(good) + + @pytest.mark.asyncio + async def test_invalid_ref_is_a_400_not_a_409(self, repo): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/x.lock"} + ) + assert resp.status == 400, await resp.text() + + @pytest.mark.asyncio + async def test_sandbox_unavailable_refuses_with_503(self, repo): + """Fail CLOSED: no OS isolation available means the git spawn does not run.""" + from kiro_crew.dashboard.handlers import worktree as wt + + with patch.object( + wt, "sandboxed_spawn_argv", side_effect=RuntimeError("no backend") + ): + async with TestClient(TestServer(_make_app(str(repo)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/nosbx"} + ) + assert resp.status == 503, await resp.text() + assert "sandbox" in (await resp.json())["error"].lower() + assert not _branch_exists(str(repo), "feat/nosbx") + + def test_git_runs_in_strict_sandbox_mode(self): + """Round 11 BLOCKING: `--includes` means `include.path` is repo-controlled, + so a hostile checkout could point it at `~/.aws/credentials` and have git + read that file as config. "standard" leaves those paths visible; strict + bind-mounts them away. Pinned so the mode cannot silently widen. + """ + from kiro_crew.dashboard.handlers import worktree as wt + + assert wt._SANDBOX_MODE == "strict" + seen: dict = {} + + def fake_spawn(argv, mode="standard", **kw): + seen["mode"] = mode + return list(argv), {}, None + + ok = subprocess.CompletedProcess(args=["git"], returncode=0, stdout="", stderr="") + with patch.object(wt, "sandboxed_spawn_argv", side_effect=fake_spawn), patch.object( + wt.subprocess, "run", return_value=ok + ): + wt._run_git(["--version"], os.getcwd()) + assert seen["mode"] == "strict" + + def test_launcher_isolation_failure_is_a_refusal_not_a_git_error(self): + """A sandbox that cannot establish isolation IN THE CHILD must not read as + a git error. + + `wrap_argv`'s backend probe passes on GitHub Actions runners, but + `unshare(NEWNS)` is denied at exec time (errno 1). git never runs, and the + non-zero exit was being reported downstream as "Not a git repository" — + a misdiagnosis that sent the user looking at their repo instead of the + host. Round 9's CI run is where this surfaced. + """ + from kiro_crew.dashboard.handlers import worktree as wt + + denied = subprocess.CompletedProcess( + args=["git"], + returncode=1, + stdout="", + stderr="sandbox: unshare(NEWNS) failed: errno 1\n", + ) + with patch.object( + wt, "sandboxed_spawn_argv", side_effect=_passthrough_spawn + ), patch.object(wt.subprocess, "run", return_value=denied): + with pytest.raises(SandboxUnavailable): + wt._run_git(["--version"], os.getcwd()) + + def test_a_real_git_failure_is_still_a_git_failure(self): + """Only the launcher's own `sandbox: ` prefix means "no isolation"; an + ordinary non-zero git exit must pass through untouched.""" + from kiro_crew.dashboard.handlers import worktree as wt + + failed = subprocess.CompletedProcess( + args=["git"], returncode=128, stdout="", stderr="fatal: not a git repository\n" + ) + with patch.object( + wt, "sandboxed_spawn_argv", side_effect=_passthrough_spawn + ), patch.object(wt.subprocess, "run", return_value=failed): + proc = wt._run_git(["status"], os.getcwd()) + assert proc.returncode == 128 + + def test_worktree_listing_survives_a_newline_in_a_path(self, repo, tmp_path): + """`--porcelain` alone splits such a path across records, so the entry never + matches and a retry 409s instead of reporting `reused`.""" + if os.name != "posix": + pytest.skip("NTFS rejects newlines in path components") + dest = tmp_path / "wt\nnewline" + assert _run_git( + ["worktree", "add", str(dest), "-b", "feat/nl", "HEAD"], str(repo) + ).returncode == 0 + trees = _worktree_branches(str(repo)) + assert trees is not None + assert trees.get(os.path.normcase(os.path.realpath(str(dest)))) == "feat/nl" + + +class TestCallerIsolation: + """Round 8 HIGH: the allow-list spans EVERY slot's project, so an app caller + reaching this endpoint could create a worktree in another app's repository.""" + + @pytest.mark.asyncio + async def test_app_caller_is_refused(self, repo): + app = _make_app(str(repo), app_claim="some-app", user="some-app") + async with TestClient(TestServer(app)) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/app"} + ) + assert resp.status == 403 + assert not (repo.parent / "proj-wt-app").exists() + assert not _branch_exists(str(repo), "feat/app") + + @pytest.mark.asyncio + async def test_non_owner_dashboard_subject_is_refused(self, repo): + """Round 12 BLOCKING: a dashboard token minted for another subject carries + `app == ""` and passed the round-8 gate, so it could create a worktree — + and a branch — in the OWNER's repository.""" + app = _make_app(str(repo), app_claim="", user="somebody-else") + async with TestClient(TestServer(app)) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/nonowner"} + ) + assert resp.status == 403 + assert not _branch_exists(str(repo), "feat/nonowner") + + @pytest.mark.asyncio + async def test_absent_auth_claim_is_refused(self, repo): + """An absent claim means the auth middleware never ran — fail closed.""" + app = _make_app(str(repo), app_claim=None) + async with TestClient(TestServer(app)) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/noauth"} + ) + assert resp.status == 403 + assert not _branch_exists(str(repo), "feat/noauth") + + +class TestResolveBaseRef: + def test_falls_back_to_head_without_remote(self, repo): + assert _resolve_base_ref(str(repo)) == "HEAD" + + +class TestAllowedRootBoundary: + """Unit coverage for the path barrier that answers the CodeQL path finding. + + Paths are built with ``os.path.join`` rather than hardcoded POSIX literals: + the matcher is ``os.sep``-based, so a ``/``-separated literal fails on + Windows for reasons that have nothing to do with the logic under test. + """ + + @staticmethod + def _p(*parts: str) -> str: + return os.path.normpath(os.path.join(os.sep + "srv", *parts)) + + def test_exact_root_returns_that_root(self): + root = self._p("repo") + assert _match_allowed_root(root, [root]) == root + + def test_descendant_returns_the_root_not_the_candidate(self): + root = self._p("repo") + assert _match_allowed_root(self._p("repo", "src", "deep"), [root]) == root + + def test_sibling_prefix_not_allowed(self): + # "…/repo-evil" shares a string prefix with "…/repo" but is a different + # directory; a naive startswith would let it through. + assert _match_allowed_root(self._p("repo-evil"), [self._p("repo")]) is None + + def test_ancestor_not_allowed(self): + assert _match_allowed_root(os.sep + "srv", [self._p("repo")]) is None + + def test_empty_root_list_denies_everything(self): + assert _match_allowed_root(self._p("repo"), []) is None + + def test_first_matching_root_wins(self): + a, b = self._p("a"), self._p("b") + assert _match_allowed_root(self._p("b", "sub"), [a, b]) == b + + +class TestRepoAllowList: + @pytest.mark.asyncio + async def test_repo_outside_slot_projects_is_denied(self, repo): + """A real git repo the caller was never granted must not be usable.""" + async with TestClient(TestServer(_make_app())) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(repo), "branch": "feat/x"} + ) + assert resp.status == 403 + assert not (repo.parent / "proj-wt-x").exists() + + @pytest.mark.asyncio + async def test_toplevel_above_every_allowed_root_is_denied(self, repo): + """Resolving upward out of an allowed subdirectory is refused. + + Only ``/src`` is granted, but git's toplevel for it is ```` + — an ancestor of the grant. The second barrier catches that. + """ + nested = repo / "src" + nested.mkdir() + async with TestClient(TestServer(_make_app(str(nested)))) as client: + resp = await client.post( + "/api/worktree/create", json={"repo": str(nested), "branch": "feat/x"} + ) + assert resp.status == 403 + assert "outside" in (await resp.json())["error"] + assert not (repo.parent / "proj-wt-x").exists() diff --git a/website/scripts/capture-followup-card.mjs b/website/scripts/capture-followup-card.mjs new file mode 100644 index 00000000000..3324db2004e --- /dev/null +++ b/website/scripts/capture-followup-card.mjs @@ -0,0 +1,231 @@ +/** + * Screenshot harness for the follow-up suggestion card. + * + * Runs the REAL built SPA (website/dist) against a static file server with every + * /api/** call and the /api/ws websocket intercepted by Playwright and answered + * from fixtures. No gateway, no dashboard token, no git, no worktrees created. + * + * The client code under test is unmodified — only the network is stubbed — so the + * card, its three actions, and the composer prefill are exercised exactly as they + * run in production. The card is driven the way the backend drives it: by pushing + * a `followup_card` frame into the live websocket after the page has rendered. + * + * Usage: node scripts/capture-followup-card.mjs + */ +import { chromium } from 'playwright' +import { mkdirSync } from 'node:fs' + +const BASE = process.argv[2] || 'http://127.0.0.1:6802' +const OUT = process.argv[3] || '../temp-screenshots/followup-suggest' +const SLOT = 'chat-followup' +const PROJECT = '/home/user/workspace/KiroCrew' + +mkdirSync(OUT, { recursive: true }) + +const slots = [{ + key: SLOT, + title: 'Add rate limiting to uploads', + running: false, + last_message: 'Added the token-bucket limiter and its tests.', + messages: 2, + agent: 'kirocrew', + memory_mode: 'persistent', + project: PROJECT, + modified: Math.floor(Date.now() / 1000), + source_links: [], + source_links_total: 0, +}] + +const detail = { + running: false, + has_more: false, + total: 2, + queue: [], + project: PROJECT, + messages: [ + { + role: 'user', + ts: Date.now() / 1000 - 600, + content: 'Add a rate limiter to the upload endpoint.', + }, + { + role: 'assistant', + ts: Date.now() / 1000 - 30, + content: + 'Added a token-bucket limiter to `POST /api/upload` plus 6 tests covering the ' + + 'refill window and the 429 path. All gates green.', + }, + ], +} + +/** The three suggestions the agent would author for that turn. */ +const ITEMS = [ + { + title: 'Add rate limiting to the WebSocket upgrade path', + description: + 'The upload endpoint is bounded now, but /api/ws still accepts unlimited concurrent upgrades from one caller.', + prompt: + 'In src/kiro_crew/dashboard/ws.py, apply the same token-bucket limiter added to the upload handler to the WebSocket upgrade path. Reuse the limiter helper rather than duplicating it, cap concurrent sockets per caller, and add tests for the reject path.', + branch: 'feat/ws-rate-limit', + }, + { + title: 'Surface the 429 in the dashboard toast', + description: + 'A rejected upload currently fails silently in the UI — the user sees nothing.', + prompt: + 'When an upload returns 429, render the retry-after value in a dashboard toast instead of failing silently. Touch website/src/api/client.ts and the upload call site, and add a vitest case.', + }, + { + title: 'Document the limiter defaults', + description: 'The new limits are undocumented, so operators cannot tune them.', + prompt: + 'Document the upload rate-limiter defaults and their config keys in src/kiro_crew/docs/configuration.md, including how to raise them for a trusted deployment.', + branch: 'docs/rate-limit-defaults', + }, +] + +/** Flipped per-scenario so one run can capture both success and failure. */ +const scene = { worktreeError: null, theme: 'dark' } + +const json = (route, body, status = 200) => route.fulfill({ + status, contentType: 'application/json', body: JSON.stringify(body), +}) + +async function main() { + const browser = await chromium.launch() + const context = await browser.newContext({ + viewport: { width: 1500, height: 950 }, + // The card is dense small type (12–13px); a 1x shot renders it soft on GitHub. + deviceScaleFactor: 2, + }) + const page = await context.newPage() + + let wsServer = null + await page.routeWebSocket(/\/api\/ws/, ws => { wsServer = ws }) + + await page.route('**/api/**', async route => { + const path = new URL(route.request().url()).pathname + if (path === '/api/worktree/create') { + if (scene.worktreeError) return json(route, { error: scene.worktreeError }, 409) + return json(route, { + ok: true, + path: PROJECT + '-wt-ws-rate-limit', + branch: 'feat/ws-rate-limit', + base: 'origin/HEAD', + }) + } + if (path === '/api/chat/slots') return json(route, slots) + if (path.startsWith('/api/chat/slots/')) return json(route, detail) + // The app shell iterates this on boot; an object-shaped stub throws inside + // the ErrorBoundary and nothing renders at all. + if (path.startsWith('/api/instances')) return json(route, { instances: [], active: '' }) + if (path === '/api/status') return json(route, { sessions: 1, crons: 0, lessons: 0, uptime: 120, version: 'dev' }) + if (path === '/api/notifications') return json(route, { notifications: [], unread: 0 }) + if (path === '/api/auth/me') return json(route, { user: 'owner', app: '' }) + if (path === '/api/models') return json(route, { models: [], default: 'auto' }) + if (path === '/api/themes') return json(route, { themes: [], installed: [] }) + if (path === '/api/theme/boot') return json(route, { mode: scene.theme, theme: '' }) + if (path === '/api/dashboard/branding') return json(route, { bot_name: 'Kiro', avatar: '' }) + if (path === '/api/recent-projects') return json(route, { dirs: [PROJECT] }) + if (path === '/api/chat/nav/resolve-links') return json(route, { summaries: [] }) + const objectish = /(config|tips|voice|autonudge|branding|status|usage-summary)/.test(path) + if (objectish) return json(route, {}) + return json(route, []) + }) + + page.on('pageerror', err => console.log('PAGEERROR:', String(err).slice(0, 300))) + page.on('console', msg => { + if (msg.type() === 'error') console.log('CONSOLE:', msg.text().slice(0, 300)) + }) + + async function load(theme) { + // The boot endpoint wins over localStorage on first paint, so the stub has + // to agree with the requested theme or every "light" shot renders dark. + scene.theme = theme + await page.addInitScript(t => { + // Wipe first: the composer persists drafts to localStorage, so without + // this the prefill scenario's text bleeds into every later scenario's + // composer and misreads as that scenario having prefilled it. + localStorage.clear() + localStorage.setItem('mc-theme', t) + localStorage.setItem('mc-onboarded', '1') + localStorage.setItem('mc-active-slot', 'chat-followup') + }, theme) + await page.goto(BASE + '/', { waitUntil: 'domcontentloaded' }) + await page.waitForTimeout(2500) + } + + /** Push the card exactly as api_chat_slot_followup broadcasts it. */ + async function pushCard(items) { + if (!wsServer) throw new Error('websocket route never bound') + wsServer.send(JSON.stringify({ + type: 'followup_card', + data: { slot: SLOT, items, ts: Date.now() / 1000 }, + })) + await page.waitForTimeout(900) + } + + async function shot(name) { + await page.screenshot({ path: `${OUT}/${name}.png` }) + console.log('wrote', `${OUT}/${name}.png`) + } + + /** Tight crop on the card + composer band, which is the whole story. */ + async function band(name) { + const card = page.getByRole('group', { name: 'Follow-up suggestions' }) + if (await card.count()) { + const box = await card.first().boundingBox() + if (box) { + await page.screenshot({ + path: `${OUT}/${name}.png`, + clip: { + x: Math.max(0, box.x - 24), + y: Math.max(0, box.y - 16), + width: Math.min(1500 - Math.max(0, box.x - 24), box.width + 48), + height: box.height + 130, + }, + }) + console.log('wrote', `${OUT}/${name}.png`) + return + } + } + await shot(name) + } + + // 1. A single suggestion — the common case, all three actions visible. + await load('dark') + await pushCard([ITEMS[0]]) + await shot('01-single-dark') + await band('02-single-dark-crop') + + // 2. Three suggestions stacked, most valuable first. + await pushCard(ITEMS) + await shot('03-three-dark') + await band('04-three-dark-crop') + + // 3. "Add to this session" pre-fills the composer and does NOT send — + // the card closes, the prompt is sitting in the input awaiting send. + await page.getByRole('button', { name: /Add to this session/ }).first().click() + await page.waitForTimeout(1200) + await shot('05-prefilled-composer-dark') + + // 4. A failed worktree create surfaces inline on the offending row instead of + // throwing, and leaves the button usable for a retry. + scene.worktreeError = 'Branch already exists: feat/ws-rate-limit' + await load('dark') + await pushCard([ITEMS[0]]) + await page.getByRole('button', { name: /Start in new worktree/ }).first().click() + await page.waitForTimeout(1500) + await band('06-worktree-error-dark-crop') + + // 5. Light-theme parity. + scene.worktreeError = null + await load('light') + await pushCard(ITEMS) + await shot('07-three-light') + await band('08-three-light-crop') + + await browser.close() +} + +main().catch(err => { console.error(err); process.exit(1) }) diff --git a/website/src/api/client.ts b/website/src/api/client.ts index 3724f050dbe..453a55e65e2 100644 --- a/website/src/api/client.ts +++ b/website/src/api/client.ts @@ -720,6 +720,17 @@ export const api = { post('/api/chat/slots/' + encodeURIComponent(slot) + '/workspace', { workspace }).then(j), chatSlotProject: (slot: string, project: string) => post('/api/chat/slots/' + encodeURIComponent(slot) + '/project', { project }).then(j), + // Follow-up card: create a sibling git worktree of `repo` on a new `branch`. + // Resolves with the created path, or rejects with the server's message + // (branch/dir already exists, not a git repo, git unavailable). + createWorktree: (repo: string, branch: string) => + post('/api/worktree/create', { repo, branch }).then(j) as Promise<{ + ok?: boolean + path?: string + branch?: string + base?: string + error?: string + }>, recentProjects: () => fetch('/api/recent-projects').then(j) as Promise<{ dirs: string[] }>, browseDirs: (path?: string) => fetch('/api/browse-dirs' + (path ? '?path=' + encodeURIComponent(path) : '')).then(j) as Promise<{ path: string; parent: string; dirs: { name: string; path: string }[] }>, browseFiles: (path?: string) => fetch('/api/browse-files' + (path ? '?path=' + encodeURIComponent(path) : '')).then(j) as Promise<{ path: string; parent: string; dirs: { name: string; path: string; mtime: number }[]; files: { name: string; path: string; mtime: number }[] }>, diff --git a/website/src/components/FollowUpCard.tsx b/website/src/components/FollowUpCard.tsx new file mode 100644 index 00000000000..b46f751a9a4 --- /dev/null +++ b/website/src/components/FollowUpCard.tsx @@ -0,0 +1,153 @@ +import { memo, useEffect, useRef, useState } from 'react' +import { GitBranch, Lightbulb, Plus, X } from 'lucide-react' +import type { FollowupItem } from '../store/chatSlice' + +export interface FollowUpCardProps { + items: FollowupItem[] + /** Pre-fill THIS session's composer with the item's expanded prompt. */ + onAddToSession: (item: FollowupItem) => void + /** + * Create a git worktree, open a session scoped to it, and pre-fill that + * session's composer. Rejects with a user-facing message on failure (branch + * exists, not a git repo, git unavailable) which the card renders inline. + */ + onStartInWorktree: (item: FollowupItem) => Promise + /** Drop this single suggestion; siblings stay. */ + onSkip: (index: number) => void + /** + * Absent when the active session has no project directory. The worktree + * button is disabled in that case — there is no repo to branch from. + */ + projectDir?: string +} + +/** + * Agent-authored follow-up suggestions, rendered above the composer. + * + * Both non-skip actions PRE-FILL a composer rather than sending: the user + * always sees the handoff prompt and presses send themselves, so a click can + * never start an unattended turn. That is a deliberate product constraint, not + * an implementation shortcut — see `suggest_followup` in mcp_core.py, whose + * tool description promises the same thing to the model. + * + * All item strings are LLM-authored. They are rendered as text children only + * (never dangerouslySetInnerHTML), on top of the server-side sanitization and + * credential/URL redaction in `_redact_followup_item`. + */ +function FollowUpCard({ + items, + onAddToSession, + onStartInWorktree, + onSkip, + projectDir, +}: FollowUpCardProps) { + // Index of the item whose worktree is being created, so only that row shows + // a pending state and double-clicks cannot fire two `worktree add` calls. + const [busyIndex, setBusyIndex] = useState(null) + const [errors, setErrors] = useState>({}) + + // Errors are keyed by array index, and Skip REMOVES an item — which shifts + // every later index down. Without this, skipping a failed item would re-render + // its neighbour under the failed item's message, misattributing the failure to + // an unrelated suggestion. Any change to `items` drops the stale errors. + // + // `itemsGen` closes the other half of the same hazard: a worktree request that + // REJECTS after `items` changed would otherwise write its error against the new + // list's index. Each request captures the generation it started in and its + // completion is ignored if that no longer matches (GPT review, round 9). + const itemsGen = useRef(0) + useEffect(() => { itemsGen.current += 1; setErrors({}) }, [items]) + + const startWorktree = async (item: FollowupItem, index: number) => { + if (busyIndex !== null) return + const gen = itemsGen.current + setBusyIndex(index) + setErrors(prev => { + const next = { ...prev } + delete next[index] + return next + }) + try { + await onStartInWorktree(item) + } catch (err) { + // Drop the error if the card's items changed under us: `index` no longer + // refers to the item this request was for. + if (itemsGen.current === gen) { + setErrors(prev => ({ + ...prev, + [index]: err instanceof Error ? err.message : 'Failed to create worktree', + })) + } + } finally { + setBusyIndex(null) + } + } + + return ( +
+
+
+ {items.map((item, index) => { + const busy = busyIndex === index + const error = errors[index] + return ( +
0 ? 'border-t border-border' : ''}`}> +
{item.title}
+ {item.description && ( +
{item.description}
+ )} +
+ + + +
+ {error && ( +
+ {error} +
+ )} +
+ ) + })} +
+ Both actions pre-fill the composer — nothing is sent until you press send. +
+
+ ) +} + +export default memo(FollowUpCard) diff --git a/website/src/hooks/useWebSocket.ts b/website/src/hooks/useWebSocket.ts index d13f5198c83..3e4eaa332c0 100644 --- a/website/src/hooks/useWebSocket.ts +++ b/website/src/hooks/useWebSocket.ts @@ -6,7 +6,7 @@ import { sseStatus, sseConnected, sseDisconnected, sseSlots, setChannelTrusted, import { addNotification, ackNotificationByTs, unackNotificationByTs, removeNotificationByTs, fetchNotifications } from '../store/notificationsSlice' import { MC_NOTIFICATION_EVENT, TURN_DONE_KIND, shouldChimeOnTurnDone, type McNotificationDetail } from './notificationEvent' import { emitThemeSound } from './themeSound' -import { fetchHistory, missedChunkMarker, sseChatMessage, sseChatMessageUpdate, sseChatMessagePatchByTs, sseThinkingChunk, refreshSlot, warmSlotCache, sseContextUsage, clearMessages, setVoicePlaying, setVoiceAudio, resolveByApprovalId, clearSubagentsForSnapshot, sseSubagentPending, sseSubagentSpawn, sseSubagentChunk, sseSubagentTool, sseSubagentStalled, sseSubagentRetrying, sseSubagentDone, sseSubagentSnapshot, sseSubagentBatchUpdate, sseSubagentBatchChunks, sseToolActivity, sseToolResult, sseActivityEvent, sseSideResult, sseWorkflowEvent, setSlotStatusDetail, removeQueuedMessage, appendQueuedMessage, cancelQueuedMessage, editQueuedMessage, appendSlotMessage, setQuestionCard } from '../store/chatSlice' +import { fetchHistory, missedChunkMarker, sseChatMessage, sseChatMessageUpdate, sseChatMessagePatchByTs, sseThinkingChunk, refreshSlot, warmSlotCache, sseContextUsage, clearMessages, setVoicePlaying, setVoiceAudio, resolveByApprovalId, clearSubagentsForSnapshot, sseSubagentPending, sseSubagentSpawn, sseSubagentChunk, sseSubagentTool, sseSubagentStalled, sseSubagentRetrying, sseSubagentDone, sseSubagentSnapshot, sseSubagentBatchUpdate, sseSubagentBatchChunks, sseToolActivity, sseToolResult, sseActivityEvent, sseSideResult, sseWorkflowEvent, setSlotStatusDetail, removeQueuedMessage, appendQueuedMessage, cancelQueuedMessage, editQueuedMessage, appendSlotMessage, setQuestionCard, setFollowupCard } from '../store/chatSlice' import { api } from '../api/client' import { sanitizeLlmOutput } from '../utils/sanitize' import { applyStatusDelta, parseStatusDelta } from '../utils/pullRequestStatusDelta' @@ -463,6 +463,28 @@ export function useWebSocket() { case 'question_card': dispatch(setQuestionCard(data as Parameters[0])) break + case 'followup_card': { + // Agent-authored follow-up suggestions. The server caps this at 3 + // items and has already sanitized + redacted every string; the + // slice keeps only the fields the card renders. + const raw = data as { slot?: string; items?: Array>; ts?: number } + const items = (Array.isArray(raw.items) ? raw.items : []) + .filter((it) => it && typeof it.title === 'string' && typeof it.prompt === 'string') + .map((it) => ({ + title: String(it.title), + description: typeof it.description === 'string' ? it.description : '', + prompt: String(it.prompt), + ...(typeof it.branch === 'string' && it.branch ? { branch: it.branch } : {}), + })) + if (raw.slot && items.length) { + dispatch(setFollowupCard({ + slot: raw.slot, + items, + ...(typeof raw.ts === 'number' ? { ts: raw.ts } : {}), + })) + } + break + } case 'activity_event': dispatch(sseActivityEvent(data as { slot: string; kind: string; text: string })) break diff --git a/website/src/pages/ChatPage.tsx b/website/src/pages/ChatPage.tsx index 29af9b7e5dc..71b08774868 100644 --- a/website/src/pages/ChatPage.tsx +++ b/website/src/pages/ChatPage.tsx @@ -18,7 +18,7 @@ import { setVoiceAudio, toggleActivity, openActivityPanel, setActiveSlot, truncateAfterIndex, replaceMessages, - requestStop, clearQuestionCard, + requestStop, clearQuestionCard, clearFollowupCard, dismissFollowupItem, } from '../store/chatSlice' import { removeNotificationByTs } from '../store/notificationsSlice' import { onTerminalReady, sendToTerminalSession } from '../utils/terminalRegistry' @@ -56,7 +56,7 @@ const SCROLL_AFTER_RENDER_MS = 100 // Canonical home is utils/navIntent (shared with the popout nav-intent // applier); re-exported here for this page's historical importers. export { PREFILL_STORAGE_KEY } from '../utils/navIntent' -import { PREFILL_STORAGE_KEY } from '../utils/navIntent' +import { PREFILL_STORAGE_KEY, writePrefill } from '../utils/navIntent' import WelcomeView from '../components/WelcomeView' import { usePanelTabs, clearInlineDraft, getInlineDraft } from '../hooks/usePanelTabs' import { useFilteredDropdown } from '../hooks/useFilteredDropdown' @@ -76,6 +76,12 @@ import SessionGridView from '../components/SessionGridView' import { anchorForSlot, loadLayout, sessionSlots } from '../hooks/splitLayoutStore' import { modelSupportsEffort } from '../lib/effort' import QuestionCard from '../components/QuestionCard' +import FollowUpCard from '../components/FollowUpCard' +import type { FollowupItem } from '../store/chatSlice' + +// Stable identity for the "no follow-up cards" case: returning a fresh {} from +// the selector would make it a new reference on every store update. +const EMPTY_FOLLOWUPS: Record = {} import ReasoningEffortDropdown from '../components/ReasoningEffortDropdown' import FlyingQuote from '../components/FlyingQuote' import { useMessageSearch } from '../hooks/useMessageSearch' @@ -542,6 +548,8 @@ export default function ChatPage({ mode, embedded, embedMode, popout }: { mode?: const slotStopping = useAppSelector(s => s.chat.slotStopping) const slotLoading = useAppSelector(s => s.chat.slotLoading) const pendingQuestion = useAppSelector(s => s.chat.pendingQuestion) + const pendingFollowup = useAppSelector(s => (s.chat.activeSlot ? s.chat.followups?.[s.chat.activeSlot] : undefined)) + const followupTsBySlot = useAppSelector(s => s.chat.followups) ?? EMPTY_FOLLOWUPS // The ambient tip yields to functional surfaces that own the above-composer band const tipSuppressed = useAppSelector(s => s.chat.messages.some(m => m.role === 'queued') || @@ -550,6 +558,10 @@ export default function ChatPage({ mode, embedded, embedMode, popout }: { mode?: // another running slot suppresses tips here forever (Codex round-31, // same slot-ownership family as the workflowRuns fix in round-16). (!!s.chat.pendingQuestion && s.chat.pendingQuestion.slot === s.chat.activeSlot) || + // The follow-up card occupies the same above-composer band. Cards are + // slot-keyed, so read only the ACTIVE slot's entry — a card parked in + // another session must not suppress tips here. + (!!s.chat.activeSlot && !!s.chat.followups?.[s.chat.activeSlot]) || // Active subagents render the progress bar in the same above-composer // zone the floating tip occupies — the tip always yields (Raymond's // hard constraint: never crowd the queue/subagent surfaces). @@ -2190,6 +2202,131 @@ export default function ChatPage({ mode, embedded, embedMode, popout }: { mode?: const tabsCtlRef = useRef(tabsCtl); tabsCtlRef.current = tabsCtl const currentProjectRef = useRef(undefined) currentProjectRef.current = currentSlot?.project || undefined + + // ── Follow-up card actions (suggest_followup MCP tool) ─────────────────── + // Both routes PRE-FILL a composer and stop; neither sends. `setPendingInput` + // is consumed by the effect above, which drops the text into the composer and + // flags the prefill hint — the same path the Projects page and command + // palette use, so there is one prefill mechanism, not a parallel one. + // + // Live per-slot card timestamps, read inside async actions without making them + // depend on (and re-create on) every card change. + const followupTsRef = useRef>({}) + followupTsRef.current = followupTsBySlot + const followupAddToSession = useCallback((item: FollowupItem) => { + if (!activeSlot) return + // APPEND when the composer already holds unsent text: the pending-input path + // replaces the draft and persists it, so a plain set would silently destroy + // whatever the user was mid-way through typing (GPT review round 13). + // `inputRef` is the live composer value; a blank line separates the two + // because a handoff prompt is multi-line prose, not a word to concatenate. + const draft = inputRef.current ?? '' + dispatch(setPendingInput(draft.trim() ? `${draft.replace(/\s+$/, '')}\n\n${item.prompt}` : item.prompt)) + // Clear by the RENDERED card's ts, as the worktree action does: a newer card + // for this slot can land between render and click, and an unqualified clear + // would delete suggestions the user never saw. + dispatch(clearFollowupCard({ slot: activeSlot, ts: followupTsRef.current[activeSlot]?.ts })) + }, [dispatch, activeSlot]) + + // Fallback branch name when the agent did not supply one: slugify the title + // under FOLLOWUP_BRANCH_RE's grammar (the server re-validates, so a slug that + // degenerates to empty is replaced rather than sent and rejected). + const followupBranchFor = useCallback((item: FollowupItem) => { + if (item.branch) return item.branch + const slug = item.title + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, '') + .slice(0, 40) + return `followup/${slug || 'suggestion'}` + }, []) + + const followupStartInWorktree = useCallback(async (item: FollowupItem) => { + const repo = currentSlot?.project + if (!repo) throw new Error('This session has no project directory to branch from.') + const originSlot = activeSlot + // Capture the card's ts up front so completion clears only THIS card. A + // newer card can arrive for the same slot while the request is in flight; + // without the guard the older action's completion would clobber it. + const originTs = originSlot ? followupTsRef.current[originSlot]?.ts : undefined + // Create the worktree FIRST: if git refuses (branch exists, not a repo), + // we must not have already spawned an empty session the user has to clean + // up. The card surfaces the thrown message inline. + const res = await api.createWorktree(repo, followupBranchFor(item)) + const path = res?.path + if (!path) throw new Error(res?.error || 'Worktree creation returned no path') + let slotKey = '' + try { + // `activate: false` on purpose: the slot must be SCOPED to the worktree + // before the user can type into it. Activating first (the default) leaves a + // window where the composer is live but `chatSlotProject` is still pending, + // so a turn sent in that window would run in the default directory — agent + // tools writing to the wrong checkout. It also means a scoping failure can + // render its error on the still-mounted card instead of unmounting it + // (GPT review, PR #461 round 9). + const slot = await dispatch(createSlot({ mode, project: path, activate: false })).unwrap() + slotKey = slot?.key || '' + } catch { + // The worktree exists but the session does not. Say so, and name the path: + // the create endpoint is idempotent for its own destination, so pressing + // the button again reuses this worktree instead of 409-ing on it. + throw new Error( + `Worktree created at ${path}, but its session could not be opened and scoped. ` + + 'Press the button again to retry — the existing worktree will be reused.', + ) + } + // A fulfilled thunk with no key would skip every guard below (scoping, + // activation, focus verification) and prefill whatever session is on screen + // — the exact fail-open the docs promise not to do. Fail closed instead. + if (!slotKey) { + throw new Error( + `Worktree created at ${path}, but no session was returned. ` + + 'Press the button again to retry — the existing worktree will be reused.', + ) + } + // Scoping is NOT done here: `createSlot({ activate: false })` awaits the + // project assignment before it publishes the slot, and deletes the session if + // that fails, so the slot is never reachable in an unscoped state. A failure + // therefore rejects the thunk and is reported by the catch above. + // createSlot's fulfilled reducer deliberately does NOT activate its result + // if the user switched sessions while the create was in flight. The + // prefill below writes to the *active* composer, so without this the + // prompt would land in whatever unrelated session is on screen and the new + // worktree session would open empty. The user asked for this worktree by + // clicking; take them to it — and if that fails, surface the error and + // keep the card rather than prefilling the wrong conversation. + // Read the store directly, NOT activeSlotRef: the ref is refreshed by a + // render, and `unwrap()` resolves as soon as the reducer ran — so a stale + // ref would report a failure (and skip the prefill) on a switch that in + // fact succeeded. store.getState() sees the committed value immediately. + // Hand the prompt over through PREFILL_STORAGE_KEY *before* the switch — the + // same channel the ?sid / popout paths use. `setPendingInput` alone loses the + // race: its consuming effect is declared BEFORE the per-slot draft-restore + // effect, so when the switch and the prefill land in one React commit the + // restore runs last and overwrites the composer with the incoming slot's + // (empty) draft, and the prompt vanishes. Seeding the prefill makes the + // restore itself apply the prompt, so there is nothing left to race. + writePrefill(slotKey, item.prompt) + if (store.getState().chat.activeSlot !== slotKey) { + try { + await dispatch(switchSlot(slotKey)).unwrap() + } catch { + throw new Error( + `Worktree ready at ${path}, but its session could not be opened. ` + + 'Switch to it in the sidebar, or press the button again.', + ) + } + } + if (store.getState().chat.activeSlot !== slotKey) { + throw new Error( + `Worktree ready at ${path}, but its session is not in focus. ` + + 'Switch to it in the sidebar, or press the button again.', + ) + } + dispatch(setPendingInput(item.prompt)) + if (originSlot) dispatch(clearFollowupCard({ slot: originSlot, ts: originTs })) + }, [currentSlot?.project, followupBranchFor, dispatch, mode, activeSlot]) + // "Run in terminal" (from chat code blocks): open a FRESH terminal tab in // this chat and run the command in it, starting in the chat's working dir. // The result is echoed back so the code-block button can show sent/failed. @@ -3515,6 +3652,17 @@ export default function ChatPage({ mode, embedded, embedMode, popout }: { mode?: /> )} + {pendingFollowup && activeSlot && ( +
+ dispatch(dismissFollowupItem({ slot: activeSlot, index, ts: pendingFollowup.ts }))} + /> +
+ )} pendingQuestion: { slot: string; questions: Array<{ question: string; header?: string; options: Array<{ label: string; description?: string }>; multiSelect?: boolean }> } | null + // Agent-authored follow-up suggestions (suggest_followup MCP tool), rendered + // as a card above the composer. Keyed BY SLOT: a single global card let a + // suggestion arriving in session B silently evict session A's unacted-on card, + // contradicting the documented per-session behaviour (GPT review, PR #461). + // + // `ts` is the broadcast timestamp, used to avoid clearing a card that arrived + // while a slower action (worktree create) was still in flight. + // + // Ephemeral: this lives only in frontend state, so a full page reload drops it. + // Deliberately NOT cleared by clearSlotState — a suggestion is not tied to an + // in-flight turn, so tabbing away and back should still show it. Rendering is + // gated on the active slot's own key, so a retained card can never surface + // under the wrong session. + followups: Record // Slot with a locally-started turn awaiting server confirmation. While set, // the slots-sync ignores a server running=false for it (the snapshot may // predate the send). Cleared on server confirmation or turn end. @@ -230,6 +263,7 @@ const initialState: ChatState = { slotSideClosed: {}, slotHistory: [], pendingQuestion: null, + followups: {}, stopPressedAt: {}, pendingTurnSlot: null, } @@ -498,8 +532,8 @@ export const warmSlotCache = createAsyncThunk( export const createSlot = createAsyncThunk< ChatSlot, - { agent?: string; model?: string; mode?: string; memory_mode?: string; clean_mode?: boolean; folder_id?: string | null; color_index?: number | null; project?: string | null } | string | undefined, - { fulfilledMeta: { originActiveSlot: string | null } } + { agent?: string; model?: string; mode?: string; memory_mode?: string; clean_mode?: boolean; folder_id?: string | null; color_index?: number | null; project?: string | null; activate?: boolean } | string | undefined, + { fulfilledMeta: { originActiveSlot: string | null; activate: boolean } } >( 'chat/createSlot', async (opts, { dispatch, getState, fulfillWithValue }) => { @@ -511,6 +545,11 @@ export const createSlot = createAsyncThunk< const folderId = typeof opts === 'string' ? undefined : opts?.folder_id const explicitColor = typeof opts === 'string' ? undefined : opts?.color_index const project = typeof opts === 'string' ? undefined : opts?.project + // `activate: false` creates the session WITHOUT stealing focus, so a caller + // that must finish setting the slot up (e.g. scoping it to a worktree) can + // do so before the user is able to type into it. Defaults to true — every + // existing caller keeps the create-and-focus behaviour. + const activate = typeof opts === 'string' ? true : opts?.activate !== false // Capture the active slot BEFORE the (potentially slow) create round-trip. // The fulfilled reducer compares this against the active slot at resolution // time: if the user switched to a different session while the create was @@ -540,14 +579,29 @@ export const createSlot = createAsyncThunk< // create payload instead.) if (project) { slot.project = project - api.chatSlotProject(slot.key, project).catch(() => {}) + if (activate) { + api.chatSlotProject(slot.key, project).catch(() => {}) + } else { + // Background create (activate: false): the caller is setting this slot up + // and the user must not be able to reach it half-configured. Publishing it + // via addSlotOptimistic makes it selectable from the sidebar immediately, + // so a turn sent before scoping landed would run in the DEFAULT checkout. + // Await the scope, and if it fails delete the session server-side rather + // than publish an unscoped one (GPT review, PR #461 round 10). + try { + await api.chatSlotProject(slot.key, project) + } catch (err) { + await api.deleteChatSlot(slot.key).catch(() => {}) + throw err + } + } } dispatch(addSlotOptimistic(slot)) // Carry the origin slot in the action meta (fulfillWithValue) rather than on // the payload, so it can never leak into the persisted slot object. The // fulfilled reducer reads action.meta.originActiveSlot to decide whether // activating the new slot is safe. - return fulfillWithValue(slot, { originActiveSlot }) + return fulfillWithValue(slot, { originActiveSlot, activate }) }, ) @@ -731,6 +785,43 @@ const chatSlice = createSlice({ setPendingInput(state, action: PayloadAction) { state.pendingInput = action.payload }, setQuestionCard(state, action: PayloadAction) { state.pendingQuestion = action.payload }, clearQuestionCard(state) { state.pendingQuestion = null }, + setFollowupCard(state, action: PayloadAction<{ slot: string; items: FollowupItem[]; ts?: number }>) { + const { slot, items, ts } = action.payload + if (!slot || !items?.length) return + if (isUnsafeKey(slot)) return // never index a state map with __proto__/constructor/prototype + // Defensive: a partial preloaded slice (tests, older persisted state) can + // arrive without this key. + if (!state.followups) state.followups = {} + state.followups[slot] = { items, ts: ts ?? Date.now() / 1000 } + }, + // `ts` guards the async case: "Start in new worktree" clears the card only + // after its request resolves, and a NEWER card may have arrived for the same + // slot meanwhile. Passing the ts the action started with means the newer card + // survives instead of being clobbered by the older action's completion. + clearFollowupCard(state, action: PayloadAction<{ slot: string; ts?: number }>) { + const { slot, ts } = action.payload + if (isUnsafeKey(slot)) return + const card = state.followups?.[slot] + if (!card) return + if (ts != null && card.ts !== ts) return + delete state.followups[slot] + }, + // Skip ONE suggestion without discarding the others. The card disappears + // only once its last item is gone, so skipping the first of three does not + // silently throw away the other two. + dismissFollowupItem(state, action: PayloadAction<{ slot: string; index: number; ts?: number }>) { + const { slot, index, ts } = action.payload + if (isUnsafeKey(slot)) return + const card = state.followups?.[slot] + if (!card) return + // Same staleness guard as `clearFollowupCard`: a replacement card can land + // between render and click, and an unqualified dismiss would delete that + // index from a card the user has not seen (GPT review, round 9). + if (ts != null && card.ts !== ts) return + const items = card.items.filter((_, i) => i !== index) + if (items.length) state.followups[slot] = { ...card, items } + else delete state.followups[slot] + }, sseContextUsage(state, action: PayloadAction<{ slot: string; pct: number; used_tokens?: number; window_tokens?: number }>) { const { slot, pct, used_tokens, window_tokens } = action.payload if (isUnsafeKey(slot)) return @@ -1591,6 +1682,9 @@ const chatSlice = createSlice({ state.slotMessages, state.slotActivity, state.slotRun, state.slotHydrated, state.slotSide, state.slotSideClosed, state.slotStatusDetail, state.slotContextPct, state.slotContextTokens, state.stopPressedAt, + // Follow-up cards are per slot and can hold multi-KB prompts, so a + // deleted session's card must not outlive it (GPT review round 4). + state.followups, ].filter(Boolean) const cached = new Set(maps.flatMap(m => Object.keys(m))) for (const key of cached) { @@ -1821,6 +1915,9 @@ const chatSlice = createSlice({ // stays put. "First create wins" rather than the prior "last wins". Both // slots exist in the sidebar and both land the user on an empty chat, so // the outcomes are equivalent, accepted over re-stealing focus. + // Caller asked for a background create (see `activate` above): the slot + // is registered but focus stays put until the caller switches to it. + if (action.meta.activate === false) return const origin = action.meta.originActiveSlot ?? null if (state.activeSlot !== origin) return if (state.activeSlot) { @@ -1845,6 +1942,7 @@ const chatSlice = createSlice({ delete state.slotHydrated[action.payload] delete state.slotSide[action.payload] delete state.slotSideClosed[action.payload] + if (state.followups) delete state.followups[action.payload] state.slotHistory = state.slotHistory.filter(k => k !== action.payload) if (state.activeSlot === action.payload) { state.activeSlot = null @@ -1901,7 +1999,7 @@ const chatSlice = createSlice({ }) export const { - setActiveSlot, clearSlotState, setPendingInput, setQuestionCard, clearQuestionCard, appendMessage, appendSlotMessage, updateStreamingMessage, finalizeAssistant, + setActiveSlot, clearSlotState, setPendingInput, setQuestionCard, clearQuestionCard, setFollowupCard, clearFollowupCard, dismissFollowupItem, appendMessage, appendSlotMessage, updateStreamingMessage, finalizeAssistant, removeThinking, removeByApprovalId, resolveByApprovalId, clearPendingPermissions, setSlotRunning, setSlotStopping, startLocalTurn, syncSlotRunningFromServer, setSlotState, setSlotStatusDetail, setStopPressedAt, clearMessages, truncateAfterIndex, replaceMessages, hydrateSlotMessages, sseChatMessage, sseChatMessageUpdate, sseChatMessagePatchByTs, sseThinkingChunk, removeQueuedMessage, appendQueuedMessage, cancelQueuedMessage, editQueuedMessage, sseContextUsage, setVoicePlaying, setVoiceAudio, toggleActivity, openActivityToTab, openActivityPanel, openActivityToTool, clearFocusToolCallId, clearSubagentsForSnapshot, sseSubagentPending, markSubagentApproving, sseSubagentSpawn, sseSubagentChunk, sseSubagentTool, sseSubagentStalled, sseSubagentRetrying, sseSubagentDone, diff --git a/website/src/test/ChatPageFollowup.test.tsx b/website/src/test/ChatPageFollowup.test.tsx new file mode 100644 index 00000000000..8df1ae17252 --- /dev/null +++ b/website/src/test/ChatPageFollowup.test.tsx @@ -0,0 +1,226 @@ +/** + * ChatPage-level orchestration for the follow-up card's worktree action. + * + * The card component and the reducers are covered in FollowUpCard.test.tsx; what + * is only reachable from the page is the ORDER and the failure handling of the + * multi-step handoff (GPT review, PR #461 round 8): create the worktree, open a + * session, scope it to the new directory, activate it, and only then prefill — + * with the session deleted again if scoping fails, so a retry cannot accumulate + * wrongly-scoped sessions. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import type { ReactNode } from 'react' +import { render, screen, fireEvent, act, waitFor } from '@testing-library/react' +import type { RootState } from '../store' +import { Provider } from 'react-redux' +import { MemoryRouter } from 'react-router-dom' +import { configureStore } from '@reduxjs/toolkit' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { ThemeProvider } from '../hooks/useTheme' +import chatReducer from '../store/chatSlice' +import dashboardReducer from '../store/dashboardSlice' +import notificationsReducer from '../store/notificationsSlice' + +vi.mock('react-virtuoso', () => ({ + Virtuoso: ({ data, itemContent }: { data?: unknown[]; itemContent: (index: number, item: unknown) => ReactNode }) => ( +
{data?.map((d: unknown, i: number) =>
{itemContent(i, d)}
)}
+ ), +})) +vi.mock('../api/client', () => ({ + api: { + // Both slots: the created one must be listed or `switchSlot` cannot activate it. + chatSlots: vi.fn().mockResolvedValue([ + { key: 'chat-1', messages: 1, running: false, mode: '', project: '/repo' }, + { key: 'chat-2', messages: 0, running: false, mode: '', project: '/repo-wt-limits' }, + ]), + chatSlotDetail: vi.fn().mockResolvedValue({ messages: [], running: false, has_more: false, total: 0 }), + sendChat: vi.fn().mockResolvedValue({ ok: true, json: () => Promise.resolve({ ok: true }) }), + chatHistory: vi.fn().mockResolvedValue({ sessions: [] }), + models: vi.fn().mockResolvedValue([]), + agents: vi.fn().mockResolvedValue([]), + agentDetail: vi.fn().mockResolvedValue({}), + workspaces: vi.fn().mockResolvedValue({ workspaces: [] }), + slackChannels: vi.fn().mockResolvedValue([]), + spawnList: vi.fn().mockResolvedValue({ agents: [] }), + uploadFiles: vi.fn().mockResolvedValue({ paths: [] }), + screenshot: vi.fn().mockResolvedValue({ path: null }), + createChatSlot: vi.fn().mockResolvedValue({ key: 'chat-2', title: 'chat-2', messages: 0, running: false }), + deleteChatSlot: vi.fn().mockResolvedValue({ ok: true }), + setSlotColor: vi.fn().mockResolvedValue({ ok: true }), + setSlotFolder: vi.fn().mockResolvedValue({ ok: true }), + chatSlotProject: vi.fn().mockResolvedValue({ ok: true }), + createWorktree: vi.fn().mockResolvedValue({ ok: true, path: '/repo-wt-limits', branch: 'followup/add-rate-limits' }), + }, + SEARCH_MIN_CHARS: 2, +})) +vi.mock('../hooks/useVoiceInput', () => ({ useVoiceInput: () => ({ recording: false, transcribing: false, toggle: vi.fn() }), voiceInputSupported: false })) +vi.mock('../hooks/useBranding', () => ({ useBranding: () => ({ botName: 'Test', avatar: '' }) })) +vi.mock('../hooks/useAgents', () => ({ useAgents: () => ({ agents: [], defaultAgent: 'default' }) })) +vi.mock('../components/MarkdownRenderer', () => ({ default: ({ content }: { content: string }) => {content} })) +vi.mock('../components/WelcomeView', () => ({ default: () => null })) +vi.mock('../components/MarkdownPanel', () => ({ default: () => null })) +vi.mock('../pages/chat/ActivityViewer', () => ({ default: () => null })) +vi.mock('../components/DetailPanel', () => ({ default: () => null })) +vi.mock('../hooks/useWebSocket', () => ({ useWebSocket: () => ({ subscribeLogs: () => {} }) })) + +Object.defineProperty(window, 'matchMedia', { + writable: true, + value: vi.fn().mockReturnValue({ matches: false, addEventListener: vi.fn(), removeEventListener: vi.fn() }), +}) + +import ChatPage from '../pages/ChatPage' +import { api } from '../api/client' + +const ITEM = { + title: 'Add rate limits', + description: 'The upload endpoint is unbounded.', + prompt: 'Add a token-bucket limiter to POST /api/upload.', +} + +function makeStore() { + return configureStore({ + reducer: { dashboard: dashboardReducer, chat: chatReducer, notifications: notificationsReducer }, + preloadedState: { + dashboard: { + status: null, connected: true, + slots: [{ key: 'chat-1', messages: 1, running: false, mode: '', project: '/repo', pending_approval: false, waiting_for_input: false, last_activity_ts: undefined }], + unreadSlots: [], refreshTrigger: 0, approvalMode: 'normal', + subagentRunning: {}, subagentDetails: {}, subagentText: {}, + } as unknown as RootState['dashboard'], + chat: { + activeSlot: 'chat-1', messages: [{ role: 'assistant', content: 'hi', cls: '' }], + slotRunning: false, slotStopping: false, slotState: 'idle', + history: [], historyHasMore: false, pendingInput: null, + subagents: {}, toolLog: [], activityOpen: false, activityTab: 'tools', + slotHasMore: false, slotOldestIndex: 0, loadingOlder: false, + slotStatusDetail: {}, slotContextPct: {}, slotActivity: {}, slotHistory: [], + historyOffset: 0, _wsChunkedDuringFetch: false, + slotMessages: {}, slotLoading: false, + followups: { 'chat-1': { items: [ITEM], ts: 100 } }, + } as unknown as RootState['chat'], + notifications: { items: [] } as unknown as RootState['notifications'], + }, + }) +} + +async function renderPage(store: ReturnType) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + await act(async () => { + render( + + + + + + + , + ) + }) + await waitFor(() => expect(screen.getByText('Add rate limits')).toBeTruthy()) +} + +beforeEach(() => { + sessionStorage.clear() + localStorage.clear() + vi.clearAllMocks() + ;(api.createWorktree as ReturnType).mockResolvedValue({ ok: true, path: '/repo-wt-limits', branch: 'followup/add-rate-limits' }) + ;(api.createChatSlot as ReturnType).mockResolvedValue({ key: 'chat-2', title: 'chat-2', messages: 0, running: false }) + ;(api.chatSlotProject as ReturnType).mockResolvedValue({ ok: true }) +}) + +describe('ChatPage follow-up worktree orchestration', () => { + const composer = () => screen.getByLabelText('Message input') as HTMLTextAreaElement + + it('creates the worktree, scopes the new session, then hands the prompt to it', async () => { + const store = makeStore() + await renderPage(store) + fireEvent.click(screen.getByRole('button', { name: /start in new worktree/i })) + // Worktree BEFORE session: a git refusal must not leave an empty session. + await waitFor(() => expect(api.createWorktree).toHaveBeenCalledWith('/repo', 'followup/add-rate-limits')) + await waitFor(() => expect(api.chatSlotProject).toHaveBeenCalledWith('chat-2', '/repo-wt-limits')) + expect((api.createWorktree as ReturnType).mock.invocationCallOrder[0]) + .toBeLessThan((api.createChatSlot as ReturnType).mock.invocationCallOrder[0]) + // The prompt is handed to the NEW slot through the prefill channel the + // slot-restore effect reads — asserting the handoff target rather than a + // rendered composer, which has several other drivers in this harness (the + // rendered value is covered by the "Add to this session" case below and by + // the Playwright capture harness). Activation is asserted separately, in the + // scoping-order test above. + // The prompt lands in the NEW session's composer: seeded into the prefill + // channel before the switch, then applied (and cleared) by the slot-restore + // effect when that slot activates. + await waitFor(() => expect(store.getState().chat.activeSlot).toBe('chat-2')) + await waitFor(() => expect(composer().value).toBe(ITEM.prompt)) + expect(sessionStorage.getItem('kirocrew_prefill')).toBeNull() + // NOT asserted here: the final card-clear. It runs after `switchSlot(...)` + // resolves, and that thunk does not settle under this harness (it wants + // hydration machinery the page mock does not provide), so asserting it would + // be asserting the harness. The clear IS asserted in the "Add to this + // session" case below, which takes the same final branch. + }) + + it('does not activate the new session until scoping has completed', async () => { + // Round 9 HIGH: createSlot used to activate immediately, so the composer went + // live while chatSlotProject was still pending — a turn sent in that window + // would run in the DEFAULT directory, not the worktree. + let releaseScope: (() => void) | undefined + ;(api.chatSlotProject as ReturnType).mockImplementation( + () => new Promise(res => { releaseScope = () => res() }), + ) + const store = makeStore() + await renderPage(store) + fireEvent.click(screen.getByRole('button', { name: /start in new worktree/i })) + await waitFor(() => expect(api.chatSlotProject).toHaveBeenCalled()) + // Scoping is still in flight: the ORIGIN session is still active AND the new + // slot is not published yet, so it cannot be selected from the sidebar and + // sent to while its CWD is still the default checkout (round 10 HIGH). + expect(store.getState().chat.activeSlot).toBe('chat-1') + expect(store.getState().dashboard.slots.map(s => s.key)).not.toContain('chat-2') + releaseScope?.() + await waitFor(() => expect(store.getState().chat.activeSlot).toBe('chat-2')) + }) + + it('deletes the session it just made when scoping fails, and keeps the card', async () => { + ;(api.chatSlotProject as ReturnType).mockRejectedValue(new Error('nope')) + const store = makeStore() + await renderPage(store) + fireEvent.click(screen.getByRole('button', { name: /start in new worktree/i })) + await waitFor(() => expect(api.deleteChatSlot).toHaveBeenCalledWith('chat-2')) + // The failed session was never published, so no unscoped slot is left behind. + expect(store.getState().dashboard.slots.map(s => s.key)).not.toContain('chat-2') + // No prefill into the wrong composer, and the suggestion survives for a retry. + expect(composer().value).toBe('') + expect(store.getState().chat.followups['chat-1']).toBeDefined() + }) + + it('surfaces a worktree failure without creating a session', async () => { + ;(api.createWorktree as ReturnType).mockRejectedValue(new Error('Branch already exists: followup/limits')) + const store = makeStore() + await renderPage(store) + fireEvent.click(screen.getByRole('button', { name: /start in new worktree/i })) + await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent(/Branch already exists/i)) + expect(api.createChatSlot).not.toHaveBeenCalled() + expect(composer().value).toBe('') + expect(store.getState().chat.followups['chat-1']).toBeDefined() + }) + + it('"Add to this session" prefills without touching git', async () => { + const store = makeStore() + await renderPage(store) + fireEvent.click(screen.getByRole('button', { name: /add to this session/i })) + await waitFor(() => expect(composer().value).toBe(ITEM.prompt)) + expect(api.createWorktree).not.toHaveBeenCalled() + expect(store.getState().chat.followups['chat-1']).toBeUndefined() + }) + + it('appends to an unsent draft instead of destroying it', async () => { + // Round 13 BLOCKING: the pending-input path replaces AND persists the draft, + // so a plain set discarded whatever the user was mid-way through typing. + const store = makeStore() + await renderPage(store) + fireEvent.change(composer(), { target: { value: 'half-written thought' } }) + fireEvent.click(screen.getByRole('button', { name: /add to this session/i })) + await waitFor(() => expect(composer().value).toContain(ITEM.prompt)) + expect(composer().value).toBe(`half-written thought\n\n${ITEM.prompt}`) + }) +}) diff --git a/website/src/test/FollowUpCard.test.tsx b/website/src/test/FollowUpCard.test.tsx new file mode 100644 index 00000000000..1009fb8734e --- /dev/null +++ b/website/src/test/FollowUpCard.test.tsx @@ -0,0 +1,250 @@ +import { describe, it, expect, vi } from 'vitest' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import FollowUpCard from '../components/FollowUpCard' +import reducer, { setFollowupCard, clearFollowupCard, dismissFollowupItem, deleteSlot } from '../store/chatSlice' +import { sseSlots } from '../store/dashboardSlice' +import type { FollowupItem } from '../store/chatSlice' + +const item = (over: Partial = {}): FollowupItem => ({ + title: 'Add rate limiting', + description: 'The upload endpoint is unbounded.', + prompt: 'Add a token-bucket limiter to POST /api/upload.', + ...over, +}) + +function setup(props: Partial> = {}) { + const onAddToSession = vi.fn() + const onStartInWorktree = vi.fn().mockResolvedValue(undefined) + const onSkip = vi.fn() + const utils = render( + , + ) + return { onAddToSession, onStartInWorktree, onSkip, ...utils } +} + +describe('FollowUpCard', () => { + it('renders the title, description and all three actions', () => { + setup() + expect(screen.getByText('Add rate limiting')).toBeInTheDocument() + expect(screen.getByText('The upload endpoint is unbounded.')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /start in new worktree/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /add to this session/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /skip/i })).toBeInTheDocument() + }) + + it('states that nothing is sent without the user pressing send', () => { + setup() + expect(screen.getByText(/nothing is sent until you press send/i)).toBeInTheDocument() + }) + + it('calls onAddToSession with the item', () => { + const { onAddToSession } = setup() + fireEvent.click(screen.getByRole('button', { name: /add to this session/i })) + expect(onAddToSession).toHaveBeenCalledWith(item()) + }) + + it('passes the item index to onSkip so siblings survive', () => { + const onSkip = vi.fn() + render( + , + ) + fireEvent.click(screen.getAllByRole('button', { name: /skip/i })[1]) + expect(onSkip).toHaveBeenCalledWith(1) + }) + + it('disables the worktree action when the session has no project dir', () => { + setup({ projectDir: undefined }) + expect(screen.getByRole('button', { name: /start in new worktree/i })).toBeDisabled() + // The in-session route stays available — it needs no repo. + expect(screen.getByRole('button', { name: /add to this session/i })).not.toBeDisabled() + }) + + it('renders the worktree failure inline instead of throwing', async () => { + const onStartInWorktree = vi.fn().mockRejectedValue(new Error('Branch already exists: feat/x')) + setup({ onStartInWorktree }) + fireEvent.click(screen.getByRole('button', { name: /start in new worktree/i })) + await waitFor(() => { + expect(screen.getByRole('alert')).toHaveTextContent('Branch already exists: feat/x') + }) + // Button is usable again so the user can retry after fixing the branch. + expect(screen.getByRole('button', { name: /start in new worktree/i })).not.toBeDisabled() + }) + + it('does not fire a second worktree call while one is in flight', async () => { + let release: (() => void) | undefined + const onStartInWorktree = vi.fn(() => new Promise(res => { release = res })) + setup({ onStartInWorktree }) + const btn = screen.getByRole('button', { name: /start in new worktree/i }) + fireEvent.click(btn) + await waitFor(() => expect(screen.getByText(/creating worktree/i)).toBeInTheDocument()) + fireEvent.click(btn) + expect(onStartInWorktree).toHaveBeenCalledTimes(1) + release?.() + }) + + it('ignores a worktree failure that lands after the items changed', async () => { + // Round 9: the rejection would otherwise write its error against the NEW + // list's index, misattributing it to a different suggestion. + let reject: ((e: Error) => void) | undefined + const onStartInWorktree = vi.fn(() => new Promise((_res, rej) => { reject = rej })) + const a = item({ title: 'A' }) + const b = item({ title: 'B' }) + const props = { projectDir: '/repo', onAddToSession: vi.fn(), onStartInWorktree, onSkip: vi.fn() } + const { rerender } = render() + fireEvent.click(screen.getAllByRole('button', { name: /start in new worktree/i })[0]) + await waitFor(() => expect(onStartInWorktree).toHaveBeenCalled()) + rerender() + reject?.(new Error('Branch already exists')) + await waitFor(() => expect(screen.getByText('B')).toBeInTheDocument()) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('drops a failed item\'s error when the item list changes', async () => { + // Errors are keyed by array index; skipping the failed item shifts its + // sibling into that index. Without the reset, B would render under A's error. + const a = item({ title: 'A' }) + const b = item({ title: 'B' }) + const onStartInWorktree = vi.fn().mockRejectedValue(new Error('Branch already exists: feat/a')) + const { rerender } = render( + , + ) + fireEvent.click(screen.getAllByRole('button', { name: /start in new worktree/i })[0]) + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()) + rerender( + , + ) + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) + expect(screen.getByText('B')).toBeInTheDocument() + }) +}) + +describe('followup card reducers', () => { + const initial = reducer(undefined, { type: 'init' }) + + it('sets and clears a card for its own slot', () => { + const withCard = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item()], ts: 10 })) + expect(withCard.followups['chat-1'].items).toHaveLength(1) + expect(reducer(withCard, clearFollowupCard({ slot: 'chat-1' })).followups['chat-1']).toBeUndefined() + }) + + it('a card in one session does not evict another session\'s card', () => { + const a = reducer(initial, setFollowupCard({ slot: 'chat-a', items: [item({ title: 'A' })], ts: 1 })) + const both = reducer(a, setFollowupCard({ slot: 'chat-b', items: [item({ title: 'B' })], ts: 2 })) + expect(both.followups['chat-a'].items[0].title).toBe('A') + expect(both.followups['chat-b'].items[0].title).toBe('B') + }) + + it('clearing one session leaves the other intact', () => { + const a = reducer(initial, setFollowupCard({ slot: 'chat-a', items: [item()], ts: 1 })) + const both = reducer(a, setFollowupCard({ slot: 'chat-b', items: [item()], ts: 2 })) + const cleared = reducer(both, clearFollowupCard({ slot: 'chat-a' })) + expect(cleared.followups['chat-a']).toBeUndefined() + expect(cleared.followups['chat-b']).toBeDefined() + }) + + it('a stale clear does not remove a newer card for the same slot', () => { + const older = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'old' })], ts: 100 })) + const newer = reducer(older, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'new' })], ts: 200 })) + // An in-flight worktree action started against ts=100 completes now. + const after = reducer(newer, clearFollowupCard({ slot: 'chat-1', ts: 100 })) + expect(after.followups['chat-1']?.items[0].title).toBe('new') + }) + + it('a matching clear does remove the card', () => { + const withCard = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item()], ts: 100 })) + expect(reducer(withCard, clearFollowupCard({ slot: 'chat-1', ts: 100 })).followups['chat-1']).toBeUndefined() + }) + + it('dismissing one item keeps the rest', () => { + const withCard = reducer( + initial, + setFollowupCard({ slot: 'chat-1', items: [item({ title: 'A' }), item({ title: 'B' })], ts: 1 }), + ) + const after = reducer(withCard, dismissFollowupItem({ slot: 'chat-1', index: 0 })) + expect(after.followups['chat-1'].items.map(i => i.title)).toEqual(['B']) + }) + + it('dismissing the last item clears the card', () => { + const withCard = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item()], ts: 1 })) + expect(reducer(withCard, dismissFollowupItem({ slot: 'chat-1', index: 0 })).followups['chat-1']).toBeUndefined() + }) + + it('a new card replaces an unacted-on one in the SAME slot rather than stacking', () => { + const first = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'A' })], ts: 1 })) + const second = reducer(first, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'B' })], ts: 2 })) + expect(second.followups['chat-1'].items).toHaveLength(1) + expect(second.followups['chat-1'].items[0].title).toBe('B') + }) + + it('a stale dismiss does not delete an index from a newer card', () => { + // Round 9: a replacement card can land between render and Skip click; an + // unqualified dismiss would drop that index from a card never seen. + const withCard = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'A' }), item({ title: 'B' })], ts: 10 })) + const replaced = reducer(withCard, setFollowupCard({ slot: 'chat-1', items: [item({ title: 'C' })], ts: 20 })) + const stale = reducer(replaced, dismissFollowupItem({ slot: 'chat-1', index: 0, ts: 10 })) + expect(stale.followups['chat-1'].items.map(i => i.title)).toEqual(['C']) + // A matching ts still dismisses. + const fresh = reducer(replaced, dismissFollowupItem({ slot: 'chat-1', index: 0, ts: 20 })) + expect(fresh.followups['chat-1']).toBeUndefined() + }) + + it('refuses to index the map with a prototype-pollution key', () => { + // Slot names are server-normalized to [\w\-.], which PERMITS __proto__. + const own = (map: object, key: string) => Object.prototype.hasOwnProperty.call(map, key) + for (const key of ['__proto__', 'constructor', 'prototype']) { + const after = reducer(initial, setFollowupCard({ slot: key, items: [item()], ts: 1 })) + // `constructor`/`prototype` resolve through the prototype chain, so assert + // no OWN entry was written rather than an undefined read. + expect(own(after.followups, key)).toBe(false) + expect(Object.getPrototypeOf(after.followups)).toBe(Object.prototype) + // The clear/dismiss reducers must be equally inert on such a key. + expect(own(reducer(after, clearFollowupCard({ slot: key })).followups, key)).toBe(false) + expect( + own(reducer(after, dismissFollowupItem({ slot: key, index: 0 })).followups, key), + ).toBe(false) + } + }) + + it('ignores an empty item list', () => { + expect(reducer(initial, setFollowupCard({ slot: 'chat-1', items: [], ts: 1 })).followups['chat-1']).toBeUndefined() + }) + + it('drops a deleted slot\'s follow-up card', () => { + const withCard = reducer(initial, setFollowupCard({ slot: 'chat-1', items: [item()], ts: 1 })) + const after = reducer(withCard, { type: deleteSlot.fulfilled.type, payload: 'chat-1' }) + expect(after.followups['chat-1']).toBeUndefined() + }) + + it('prunes follow-up cards for slots the server no longer lists', () => { + let s = reducer(initial, setFollowupCard({ slot: 'chat-live', items: [item({ title: 'A' })], ts: 1 })) + s = reducer(s, setFollowupCard({ slot: 'chat-gone', items: [item({ title: 'B' })], ts: 2 })) + const after = reducer(s, sseSlots([{ key: 'chat-live' }] as never)) + expect(after.followups['chat-live']).toBeDefined() + expect(after.followups['chat-gone']).toBeUndefined() + }) +}) diff --git a/website/src/test/useWebSocketFollowupCard.test.ts b/website/src/test/useWebSocketFollowupCard.test.ts new file mode 100644 index 00000000000..307c69df934 --- /dev/null +++ b/website/src/test/useWebSocketFollowupCard.test.ts @@ -0,0 +1,124 @@ +/** + * `followup_card` WebSocket frame -> store, through the real dispatch adapter. + * + * The reducers and the card component have their own suites, but the adapter in + * `useWebSocket.ts` — the code that decides which frames are well-formed enough + * to become a card — was only ever exercised indirectly (GPT review, PR #461 + * round 10). Everything the server sends is already sanitized and redacted; this + * pins the client's own shape filtering so a malformed or partial frame cannot + * put junk in the store. + */ +import { renderHook, act } from '@testing-library/react' +import { createElement } from 'react' +import { Provider } from 'react-redux' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { createTestStore } from './helpers' +import { useWebSocket } from '../hooks/useWebSocket' +import chatReducer from '../store/chatSlice' + +vi.mock('../api/client', () => ({ + api: { + chatSlots: vi.fn().mockResolvedValue([]), + voiceConfig: vi.fn().mockResolvedValue({ autoSpeak: false }), + approvals: vi.fn().mockResolvedValue([]), + notifications: vi.fn().mockResolvedValue({ notifications: [], unread: 0 }), + chatSlotDetail: vi.fn().mockResolvedValue({ messages: [], running: false, has_more: false, total: 0, queue: [] }), + }, +})) + +const WS_INSTANCES: MockWebSocket[] = [] + +class MockWebSocket { + static OPEN = 1 + static CONNECTING = 0 + readyState = MockWebSocket.CONNECTING + onopen: ((ev: Event) => void) | null = null + onmessage: ((ev: MessageEvent) => void) | null = null + onclose: ((ev: CloseEvent) => void) | null = null + onerror: ((ev: Event) => void) | null = null + send = vi.fn() + close = vi.fn() + + constructor() { WS_INSTANCES.push(this) } + + simulateOpen() { + this.readyState = MockWebSocket.OPEN + this.onopen?.(new Event('open')) + } + + simulateMessage(data: object) { + this.onmessage?.(new MessageEvent('message', { data: JSON.stringify(data) })) + } +} + +const ITEM = { + title: 'Add rate limits', + description: 'The upload endpoint is unbounded.', + prompt: 'Add a token-bucket limiter to POST /api/upload.', +} + +describe('useWebSocket followup_card frame', () => { + let testStore: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + WS_INSTANCES.length = 0 + testStore = createTestStore({ chat: { ...chatReducer(undefined, { type: '@@INIT' }), activeSlot: 'chat-1' } }) + vi.stubGlobal('WebSocket', MockWebSocket) + }) + + afterEach(() => { vi.unstubAllGlobals() }) + + function wrapper({ children }: { children: React.ReactNode }) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + return createElement(Provider, { store: testStore }, + createElement(QueryClientProvider, { client: qc }, children), + ) + } + + function send(data: object) { + renderHook(() => useWebSocket(), { wrapper }) + const ws = WS_INSTANCES[0] + act(() => { ws.simulateOpen() }) + act(() => { ws.simulateMessage({ type: 'followup_card', data }) }) + } + + it('stores a well-formed card against its own slot', () => { + send({ slot: 'chat-1', items: [ITEM], ts: 42 }) + const card = testStore.getState().chat.followups['chat-1'] + expect(card.ts).toBe(42) + expect(card.items).toEqual([{ ...ITEM, description: ITEM.description }]) + }) + + it('keeps an optional branch and defaults a missing description', () => { + send({ slot: 'chat-1', items: [{ title: 'T', prompt: 'P', branch: 'feat/x' }] }) + const item = testStore.getState().chat.followups['chat-1'].items[0] + expect(item.branch).toBe('feat/x') + expect(item.description).toBe('') + }) + + it('drops items missing the fields the card renders', () => { + send({ slot: 'chat-1', items: [{ description: 'no title or prompt' }, ITEM] }) + expect(testStore.getState().chat.followups['chat-1'].items).toHaveLength(1) + }) + + it('ignores a frame with no slot, no items, or a non-array items field', () => { + for (const data of [ + { items: [ITEM] }, + { slot: 'chat-1', items: [] }, + { slot: 'chat-1', items: 'nope' }, + { slot: 'chat-1' }, + ]) { + WS_INSTANCES.length = 0 + testStore = createTestStore({ chat: { ...chatReducer(undefined, { type: '@@INIT' }), activeSlot: 'chat-1' } }) + send(data) + expect(testStore.getState().chat.followups['chat-1']).toBeUndefined() + } + }) + + it('routes a card to a background slot without touching the active one', () => { + send({ slot: 'chat-other', items: [ITEM] }) + expect(testStore.getState().chat.followups['chat-other']).toBeDefined() + expect(testStore.getState().chat.followups['chat-1']).toBeUndefined() + }) +})