Skip to content

feat(chat): agent-suggested follow-ups as a card above the composer - #461

Merged
iamwhatever merged 1 commit into
mainfrom
feat/followup-suggest
Jul 27, 2026
Merged

feat(chat): agent-suggested follow-ups as a card above the composer#461
iamwhatever merged 1 commit into
mainfrom
feat/followup-suggest

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

When the agent finishes a turn it often knows exactly what should happen next — the sibling endpoint that still needs the same fix, the doc that is now stale, the test gap it noticed on the way through. Today that knowledge has nowhere to go but prose. The user reads it, and then has to retype it as a new prompt, from memory, usually losing the specifics the agent had in hand. Worse, work that should not share the current working tree gets started in it anyway, because opening a worktree and a fresh session is several manual steps.

Why it matters

The expensive part of a follow-up is not deciding to do it — it is reconstructing the context. Every hand-retyped follow-up is a lossy copy of an instruction the agent could have written precisely. And every follow-up done in the current tree instead of a fresh one is a branch-hygiene problem that surfaces later as an unrelated diff in someone's PR.

Fix (symptoms → root cause → change)

Symptom: the agent's proposed next steps are unactionable prose; acting on them means retyping and losing detail.

Root cause: there is no agent-callable surface that can put a structured, actionable suggestion in front of the user. The dashboard already has an above-composer card band (the question card lives there), but nothing agent-reachable writes to it. The existing QuestionCard pipeline cannot be reused: it waits on a native AskUserQuestion tool that does not exist in the kiro-cli binary, so it is dead code from the agent's side.

Change: a new suggest_followup MCP tool in kirocrew-core — agent-callable today, no dependency on unshipped CLI surface — that takes up to three {title, description, prompt, branch?} items and renders them as a card in that band.

prompt is the substance: the tool description instructs the model to write a complete, standalone handoff instruction on the assumption the next agent shares none of this session's context. title/description are only the human-facing label.

Three actions per item:

Action Effect
Start in new worktree Creates <parent>/<repo>-wt-<slug> on a new branch off the repo's default branch, opens a session scoped to it, and pre-fills that session's composer. Disabled when the session has no project directory.
Add to this session Pre-fills the current session's composer.
Skip Drops that one suggestion; siblings remain. The card clears when its last item is gone.

Both non-skip actions pre-fill and stop. Nothing is sent until the user presses send, so one click can never start an unattended turn. That constraint is enforced in the UI and promised to the model in the tool description, so it cannot quietly drift.

Worktree creation happens before the session is opened, so a git refusal (branch exists, not a repo) never leaves an orphan empty session behind — the error renders inline on the offending row and the button stays usable for a retry.

Trust model

Every string in an item is LLM-authored and one of them (branch) reaches a git invocation, so there are two independent gates:

  1. MCP layerSUGGEST_FOLLOWUP_SCHEMA caps item count and per-field 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. GatewayPOST /api/chat/slots/{slot}/followup re-validates against the same schema and redacts credentials + exfiltration URLs before broadcasting. This endpoint is reachable over loopback from inside the kiro-cli process group, so it is a trust boundary in its own right, not a relay for the MCP layer.

POST /api/worktree/create adds its own:

  • repo must resolve inside a directory an 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 — without it any authenticated dashboard caller could name any directory on the host. Both the submitted path and the git toplevel it resolves to are checked, so resolving upward out of an allowed subdirectory is refused. The prefix test is boundary-aware, so /repo-evil does not pass as being inside /repo.
  • git runs with an argv list and no shell, a credential-scrubbed environment, the POSIX resource-limit ceiling, and a 120s timeout. It is deliberately not OS-sandbox-routed, and is recorded in the spawn audit's BENIGN_SPAWNS with that justification — see "Sandbox classification" below.
  • Sensitive paths are refused; the destination is derived server-side and must not already exist.

Both endpoints emit SEL audit records.

Sandbox classification

The first revision routed git through sandboxed_spawn_argv. CI caught why that was wrong: the chokepoint raises rather than degrading when no OS sandbox backend is available, so every worktree create returned 500 on Windows and would do the same on macOS hosts without sandbox-exec. That is a product bug, not a test artifact.

git here is in the same class as the repo's existing local-VCS spawns (cloud/source.py::_git_tracked_files, pod/runtime.py::_git_worktrees): a fixed binary, a server-derived destination, a cwd already constrained to an allowed slot project, and one agent-reachable input — the branch name — that has full-matched FOLLOWUP_BRANCH_RE at two independent layers. Failing a local VCS call closed on those platforms buys no isolation the user does not already have. So it is allowlisted with that reasoning, and the protections that do apply are kept at the call site.

One note for future readers, documented in the code: the audit classifies a spawn by text-matching the enclosing function's source, so the rationale deliberately lives in the module docstring — naming the chokepoint helper inside the function would make an unrouted spawn read as routed.

The tool is dashboard-only via _resolve_session_key_strict() — Slack, cron and subagent contexts fail closed rather than posting a card into another session.

Scope notes

  • Calling the tool is the agent's judgement call. There is deliberately no turn-boundary hook forcing a suggestion every turn; the tool description makes silence the default when there is no substantive follow-up.
  • The card is ephemeral: it survives switching between sessions but not a full page reload (same posture as the question card). Persisting across reloads is a possible follow-up, not in scope here.

Tests

Backend — 49 new

  • test/test_followup_suggest.py (29): schema accepts a minimal item and an optional branch; rejects empty/oversized item lists, missing prompt, non-object items, unknown item and top-level fields, oversized titles, whitespace-only titles; accepts a prompt exactly at the limit; treats an empty branch as absent; strips a zero-width space from a title; parametrised over 10 dangerous branch names (leading --, .. traversal, whitespace, shell metacharacters, @{0}, ~1, ^, :, //, leading /). Endpoint: broadcasts the card, 404 on unknown slot, 400 on invalid JSON and on a JSON-array body, re-validates the schema independently of the MCP layer, and redacts a credential before broadcast.
  • test/test_worktree_create.py (20): runs against a real throwaway git repo. Rejects 7 unsafe branch shapes, non-string inputs, missing directories, non-git directories, invalid JSON; creates a genuine sibling worktree and asserts it is a sibling (not nested) with the base commit's files present; proves a nested path resolves to the repo toplevel; 409 on an existing branch and on an existing destination directory; covers dir-slug derivation and the origin/HEADHEAD base-ref fallback.

Frontend — 11 new (website/src/test/FollowUpCard.test.tsx): renders title/description/three actions; asserts the "nothing is sent" copy is present; onAddToSession receives the item; onSkip receives the item index so siblings survive; the worktree button is disabled without a project dir while the in-session route stays enabled; a rejected worktree renders inline via role="alert" and leaves the button retryable; a second click cannot fire while one is in flight. Plus the four reducers: set/clear, dismiss-one-keeps-rest, dismiss-last-clears, and a new card replacing an unacted-on one rather than stacking.

Manual verification

Screenshots below were captured by driving the real built SPA (website/dist) under Playwright with the network stubbed from fixtures — no gateway, no token, and no worktrees actually created. The card is driven the way the backend drives it: a followup_card frame pushed into the live websocket after the page has rendered, so the WS handler, the slice, the slot-ownership gate, and the composer prefill are all exercised as shipped. Harness committed at website/scripts/capture-followup-card.mjs for re-runs.

Not exercised end-to-end: a real git worktree add through the browser path (the endpoint's own suite covers that against a real repo, but the click-to-new-session hop is fixture-backed).

Screenshots

Three suggestions stacked, most valuable first — dark:

Three follow-up suggestions in dark theme

"Add to this session" pre-filled the composer and closed the card — note the prompt is sitting in the input awaiting send, not sent:

Composer pre-filled with the handoff prompt

A failed worktree create reports inline on its own row and leaves the button retryable:

Inline branch-already-exists error on the suggestion row

Full-page context and light-theme parity

Single suggestion, dark:

Single suggestion, dark

Single suggestion crop, dark

Three suggestions, full page dark:

Three suggestions full page, dark

Light theme:

Three suggestions full page, light

Three suggestions crop, light

Local gates

pytest 17208 passed · isort clean · flake8 clean · mypy 474 files · tsc -b clean · vitest 4508 passed / 389 files · eslint 0 errors · jscpd 0 clones · theme-colors advisory (no new literals).

Remaining local failures are pre-existing or environmental, not from this change: test_dashboard_origin::TestParseDashboardUrlMalformed (×3) and test_skills::test_flat_copy_untouched_when_nested_missing fail identically on a clean origin/main checkout on this host; a rotating test_apps_registry case and test_mcp_gateway_wedge_ping_gate::test_unknown_notification_silently_ignored pass in isolation and are load-sensitive under xdist.

Docs: src/kiro_crew/docs/followup-suggestions.md, linked from the docs index.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of b1d42b9318b421b1857f9516d65f1193776382ed and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/handlers/worktree.py:154 -- Native Windows has no sandbox backend, so "_SANDBOX_MODE = \"strict\"" makes every worktree action return 503, contradicting the stated cross-platform behavior -> Fix: use the credential-scrubbed unsandboxed mode described in the PR.
[CODEX-REVIEWED] b1d42b9

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b1d42b9318b421b1857f9516d65f1193776382ed: <one-sentence reason>

Comment thread src/kiro_crew/dashboard/handlers/worktree.py Fixed
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of b1d42b9318b421b1857f9516d65f1193776382ed — updated in place on each push; does not block merge.

This is enough to assess the design. I have the full picture: MCP tool → gateway endpoint (re-validates, redacts, owner-only broadcast) → worktree endpoint (sandbox-routed git, allow-list, atomic branch claim) → frontend card with prefill-only actions.

One thing I confirmed: _run_git fails closed (503 SandboxUnavailable) when no OS sandbox backend exists — and there is no Windows backend (only Linux userns / macOS sandbox-exec). Meanwhile the PR body's "Sandbox classification" section still describes the old design (git "deliberately not OS-sandbox-routed… recorded in BENIGN_SPAWNS"), which the code comments say was explicitly withdrawn in round 9.

Design-Verdict: CONCERNS

Sound design for a real problem, but the headline "Start in new worktree" action is dead on Windows (a first-class platform) and the PR's sandbox-classification section describes a design the code reversed.

Watch

  • Worktree route fails closed with 503 wherever no OS sandbox backend exists — i.e. all of Windows and macOS hosts without sandbox-exec. AGENTS.md treats Windows as first-class, so a primary action of the feature is unavailable there. It degrades gracefully (inline error, "Add to this session" still works), and this is a defensible fail-closed choice given the LLM-authored branch reaching git — but a human should merge knowing the worktree button is inert on an entire supported platform, not assume it's universal.

Suggestions

  • The PR body's "Sandbox classification" section contradicts the shipped code (claims git is not sandbox-routed / in BENIGN_SPAWNS; _run_git routes through sandboxed_spawn_argv and the code says that classification was withdrawn). Update the description so a future reader doesn't trust the wrong rationale — the code comments are correct, the prose is stale.

[DESIGN-REVIEWED] b1d42b9

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 3c46e32 to 2365ba9 Compare July 25, 2026 22:38
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 1 disposition — head 2365ba95

Three legitimate findings, all fixed. No rebuttals this round.

1. Backend Tests shard 4 (Linux 3.10, 3.12, Windows) — FIXED, and it was a real product bug

Six test_worktree_create cases returned 500. Root cause was not the tests: sandboxed_spawn_argv raises when no OS sandbox backend is available and allow_unsandboxed_exec is unset ("Probe detail: not Linux"), so routing git through it meant every worktree create 500s on Windows, and on any macOS host without sandbox-exec. CI surfaced a platform bug the Linux-only local gate could not.

Reclassified _run_git as a benign spawn with a written justification in BENIGN_SPAWNS, matching how this repo already treats cloud/source.py::_git_tracked_files and pod/runtime.py::_git_worktrees: fixed binary, server-derived destination, cwd constrained to an allowed slot project, and one agent-reachable input (the branch name) already full-matched against FOLLOWUP_BRANCH_RE at two layers. Sandboxing a local VCS binary the user can invoke directly buys no isolation worth failing the feature closed on two platforms for.

The applicable protections are kept at the call site and are now the only thing in that function body: argv list with no shell, scrub_env, resource_limit_preexec() (returns None on Windows by contract, where preexec_fn must be None — the previous revision would also have raised ValueError there), 120s timeout, GIT_TERMINAL_PROMPT=0.

Gotcha recorded in the code for whoever touches this next: the audit classifies a spawn by text-matching the enclosing function's source, so the rationale lives in the module docstring. Naming the chokepoint helper inside the function makes an unrouted spawn read as routed — which is exactly how test_benign_allowlist_has_no_stale_entries caught my first attempt at this fix.

2. CodeQL — uncontrolled data in path expression (worktree.py:192) — FIXED

Legitimate. The endpoint accepted any directory from an authenticated dashboard caller and ran git against it. Added an allow-list barrier: repo must resolve inside a directory some existing chat slot is already scoped to (_allowed_repo_roots). The card only ever sends the active session's own project, so this costs the feature nothing.

Two details worth review:

  • The check runs on the realpath, so a symlink or .. segment cannot alias past a root, and the prefix test is os.sep-terminated so /repo-evil does not pass as being inside /repo.
  • The git toplevel is re-checked against the same roots. Resolving upward from an allowed subdirectory can land on a repo root above every allowed root — without the second check, granting <repo>/src would have let git operate on <repo>. There is a test for exactly that.

3. Inclusive Language — FIXED

One added line in a _resolve_base_ref docstring referenced the historical default-branch name while explaining why the code reads origin/HEAD instead of hardcoding. Reworded to "repos whose default branch is named something else" — same meaning, no flagged term. (Reminder for future rounds: woke merges its built-in default ruleset with .woke.yml, so default terms trip the check even when absent from the repo config.)

Verification

New tests: 5 unit cases on the allow-list boundary (exact root, descendant, sibling-prefix rejection, ancestor rejection, empty-root-list denies) and 2 endpoint cases (repo outside all slot projects → 403 with no directory created; toplevel above every allowed root → 403). Existing worktree tests now seed slot state.

Local gates: pytest 17208 passed, isort clean, flake8 clean, mypy 474 files. Screenshot SHAs re-pinned to 2365ba95; the UI is unchanged this round.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch 2 times, most recently from 93361a0 to 0d8977e Compare July 25, 2026 23:21
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 2 disposition — head 0d8977e6

All six review findings addressed. One of them changed the design; details below.

GPT HIGH — repo-controlled hook execution (worktree.py) — FIXED at the root

Accepted, and the reasoning was better than my first answer to it. The substance of the finding is not "this call is unwrapped", it is git worktree add executes the repository's post-checkout hook, which is repo-controlled code. Wrapping that in a sandbox contains the blast radius; it does not stop the code from running, and it costs the feature entirely on Windows and on macOS without sandbox-exec (the chokepoint raises rather than degrading, which is what made CI red last round).

So the hook execution is removed instead:

git -c core.hooksPath=.git/kirocrew-no-hooks -c core.fsmonitor=false worktree add …

-c beats every config file, so this holds against a hostile .git/config. core.hooksPath points at a fixed non-existent path so no post-checkout hook is found; core.fsmonitor=false closes the other repo-config-names-a-command vector on index reads. No repository-supplied program runs at all, which removes the reason this spawn would need OS isolation rather than arguing about the wrapper.

Verified empirically before claiming it, and locked in with a two-part test: a control case asserts the hook does fire without the overrides (so the test cannot pass because the harness failed to install a working hook), and the endpoint case asserts it does not — while still checking the checkout populated correctly.

If you'd rather have the sandbox wrapper and accept that "Start in new worktree" returns a clear error on Windows and on sandbox-less macOS, that's a product call I'd want from a maintainer rather than making unilaterally — say so and I'll switch it.

GPT MEDIUM — single global card slot (chatSlice.ts) — FIXED

Legitimate, and it contradicted this PR's own documented behaviour. pendingFollowup was one global entry, so a card arriving in session B evicted session A's. Replaced with a slot-keyed followups: Record<slot, {items, ts}>; set/clear/dismiss all take a slot. Tests cover "a card in one session does not evict another's" and "clearing one leaves the other intact".

GPT MEDIUM — createSlot does not await the project assignment — FIXED

Legitimate. createSlot fires api.chatSlotProject(...).catch(() => {}) without awaiting, so the new composer could accept a turn before the CWD applied — the first turn would run in the default directory, and a failed assignment was swallowed while the UI presented a worktree-scoped session.

Fixed inside the card's own flow rather than by changing createSlot for every existing caller: followupStartInWorktree now awaits api.chatSlotProject(slotKey, path) explicitly and surfaces a specific error if it fails, instead of silently presenting a mis-scoped session. Flagging the choice deliberately — the shared thunk still has the fire-and-forget behaviour for its other callers, and tightening it there is a wider blast radius than this PR should take on.

GPT MEDIUM — orphaned worktree when slot creation fails — FIXED (idempotent re-entry)

Legitimate; the asymmetry Design review #2 also called out. Rather than add a delete endpoint (more host-mutating surface), the create endpoint is now idempotent for its own destination: if the destination is already the registered worktree for this repo on this branch, it returns success with reused: true instead of 409. The client catches slot-creation failure and tells the user the worktree exists at <path> and that pressing the button again will reuse it.

Guarded against over-reach: a directory at that path which is not a registered worktree of this repo still 409s, and there is a test asserting an unrelated squatter directory is left untouched.

GPT MEDIUM — partial artifacts on failure/timeout — FIXED

Legitimate. git worktree add can register the worktree and create the branch before failing later in the same command. Added _cleanup_partial (worktree remove → branch -D → rmtree fallback → prune), called on non-zero exit, on timeout, and on the defensive "success but no directory" path. It only ever runs where both the destination and the branch were absent before the request, so it cannot delete something that pre-existed. Test asserts a failed add leaves no branch or directory and that the retry then succeeds.

GPT MEDIUM — stale clear of a newer card — FIXED

Legitimate. The worktree action clears the card after its async work, which could clobber a card that arrived meanwhile. clearFollowupCard now takes the ts the action started with and no-ops if the slot's current card has a different one. Tests cover both the stale-clear-is-ignored and matching-clear-does-remove directions.

Design CONCERNS #1 — fire-and-forget with a false success receipt — FIXED

This was the sharpest finding in the round and I think it was right that the "same posture as the question card" analogy did not hold: a question card blocks on a live user, a follow-up card fires exactly when the user may be away.

The endpoint now reports delivered (the connected WS client count, via the existing ws_client_count()), and when it is zero the tool returns a warning telling the model the suggestions were NOT delivered and to restate them in its reply text — instead of "Showed N suggestions… End your turn now". The model is no longer steered into silence over a card nobody received. Three tests cover the count, the zero case, and that a failure to read the count degrades to zero without breaking delivery.

Server-side parking + replay on reconnect is the fuller fix; it is called out as a follow-up in the docs rather than done here, since it needs slot-state persistence semantics.

Design CONCERNS #3 — generic endpoint, single caller — NOT changed, flagged for a human

Fair contract observation and I'd rather not guess. /api/worktree/create is registered as a general route but its limits are followup-named (FOLLOWUP_BRANCH_RE, MAX_FOLLOWUP_BRANCH) and its containment is the slot-project allowlist. Two coherent answers — namespace it as the card's private backend, or own it as a general worktree API with contract-named limits — and picking is a maintainer decision about a one-way door. Happy to implement either.

Verification

New tests this round: 8 backend (hook-suppression control + endpoint, idempotent retry, squatter refusal, failed-add cleanup + retry-clears, delivery count ×3) and 8 frontend (slot isolation ×2, stale/matching clear ×2, per-slot dismiss, replacement, empty-list guard).

One self-inflicted regression caught and fixed before pushing: making the slice slot-keyed broke 63 existing frontend tests, which build a partial chat slice with no followups key — the new selectors and reducers are now optional-safe, with a stable empty-object identity so the selector does not churn.

Local gates: pytest 17217 passed, vitest 4513 passed / 389 files, isort, flake8, mypy 474 files, tsc, eslint 0 errors. Screenshots re-captured against the reworked slice and re-pinned to 0d8977e6; the card's appearance is unchanged.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Opus 5 Review — ✅ no blocking findings

Reviewed b1d42b9318b421b1857f9516d65f1193776382ed — this comment is updated in place on each push.

No findings.

Verdict recorded via the action's structured output for commit b1d42b9318b421b1857f9516d65f1193776382ed.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b1d42b9318b421b1857f9516d65f1193776382ed: <one-sentence reason>

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging b1d42b9318b421b1857f9516d65f1193776382ed.

Second-order review for b1d42b9318b421b1857f9516d65f1193776382ed; this comment is updated in place on each push.

Review details

I have the two files. There are only three sub-threshold items to judge: the GPT 5.6 finding (worktree 503 on Windows), and the design review's two CONCERNS (worktree inert on sandboxless platforms; stale PR description). Opus found nothing. Let me assess each against the narrow blocking bar.

All three describe the same underlying behavior or documentation drift, and none are one-way doors or concrete harm:

  • Worktree 503 on Windows / sandboxless hosts (GPT finding + design Watch): this is a deliberate fail-closed degradation, not a regression. The endpoint returns a clear inline error, the "Add to this session" path still works, and nothing crashes, hangs, leaks, or loses data. It touches no persisted schema or wire contract that would be costly to reverse — flipping to a credential-scrubbed unsandboxed mode later is a self-contained code change. Reversible, so it's a follow-up, not a gate.
  • Stale PR "Sandbox classification" prose: documentation-only; the shipped code is correct. Purely reversible, never blocking.

Nothing in the diff I reviewed leaks a secret or pulls in an incompatible dependency.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

  • Worktree action inert on Windows / macOS-without-sandbox-exec (GPT 5.6 line finding + Design Watch): the "Start in new worktree" action returns 503 wherever there is no OS sandbox backend, so a first-class supported platform loses the feature's headline action. It can safely wait because it degrades gracefully (clear error, "Add to this session" still works) and is a fail-closed choice a human should merge knowingly; a later change can add a credential-scrubbed unsandboxed mode or a Windows backend in src/kiro_crew/dashboard/handlers/worktree.py (_SANDBOX_MODE / _run_git).
  • Stale "Sandbox classification" section in the PR body (Design suggestion): the description still claims git is not sandbox-routed / recorded in BENIGN_SPAWNS, which the shipped _run_git reversed. Doc-only and reversible; fix by editing the PR description so a future reader doesn't trust the withdrawn rationale (the in-code comments are already correct).

[ARBITER-REVIEWED] b1d42b9

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter b1d42b9318b421b1857f9516d65f1193776382ed: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch 2 times, most recently from bf2e3cc to 8e16a44 Compare July 25, 2026 23:51
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 3 disposition — head 8e16a445

Five GPT findings, all fixed. Two changed the design; details below.

GPT HIGH — checkout content filters (worktree.py:125)

Accepted as a real residual: the -c overrides closed hooks and core.fsmonitor, but a filter.<name>.process/.smudge driver is named from .gitattributes under an arbitrary name, so -c cannot enumerate and disable it.

Rather than argue the wrapper, the vector is now refused: _local_checkout_filter() probes git config --local --name-only --list and any filter.*.{process,smudge,clean} key returns 409 with "create the worktree manually" — before the branch is claimed or anything is written.

Scope note, deliberate: only --local config is probed. Filter drivers can only come from a config file (never from .gitattributes, and git clone does not transfer config from a remote), so .git/config is the only repository-scoped source. Global config is the user's own machine setup — git lfs install writes there — and gating on it would refuse worktree creation on every LFS repo for no gain in the threat model.

GPT HIGH — concurrent requests could destroy each other's worktree

Correct, and the sequence was exactly as described. Fixed by claiming the branch before creating anything:

git update-ref --create-reflog refs/heads/<branch> <base_sha> ""

The empty old-value means "the ref must not exist", so git's ref lock picks a single winner (verified: second call exits 128 reference already exists) and the losers get a 409. worktree add <dest> <branch> then checks out the ref we own instead of creating it with -b.

Cleanup now removes only what the request can prove it created:

  • the branch — only when _claim_branch returned True for this request;
  • the destination — only when worktree list --porcelain registers it against this branch, or when it exists registered to nothing (our own add's leftover).

Plus _repo_lock(), one asyncio.Lock per repo root, serializing same-repo requests in-process. That is not what makes the claim atomic — git is — but it closes the same-destination window between the "does dest exist" probe and worktree add, which is what makes the "registered to nothing ⇒ ours" inference safe.

GPT MEDIUM — same slug, different branch reported as reused

Real. _dir_slug keeps only the branch's last segment, so feat/foo and fix/foo derive the same directory, and the old reuse test was path registered + branch exists anywhere. Both could pass for a different branch's worktree, and the session would open on the wrong tree.

_is_registered_worktree is replaced by _worktree_branches(), which parses the porcelain blocks into path -> branch. Reuse now requires trees[dest] == branch; anything else is a 409.

GPT MEDIUM — prompt prefill landing in an unrelated session

Real. createSlot.fulfilled deliberately declines to activate its result if the user switched sessions mid-flight, but the prefill writes to the active composer — so switching during creation put the handoff prompt in the session on screen and left the new worktree session empty.

The handler now activates the created slot (switchSlot) before prefilling when it is not already active. Chosen over slot-keyed pending input because the user asked for this worktree by clicking; landing them on it is the coherent outcome.

GPT MEDIUM — orphaned session when project assignment fails

Real. chatSlotProject failing left a created, unscoped session behind, and retry (which reuses the worktree) accumulated more. The failure path now deleteSlots the session it just made and says "press the button again to retry — the existing worktree will be reused".

Windows shard 4 — test_signing_secret_persisted_across_loads

Not addressed and not believed to be from this diff: the test asserts a signing secret persists across two _load_or_create_secret() calls, in test_token_auth.py, which this branch does not touch. Main's Windows shards are green, so it is not a standing red — most likely load/ordering sensitivity surfaced by this branch's two new test files shifting shard composition. This push re-runs it; if it recurs deterministically I will treat it as a real Windows bug in the secret load path rather than a shard artifact.

Gates

pytest 17223 passed (excluding 3 test_dashboard_origin env failures that reproduce on clean main on this host, and 1 load-sensitive test_apps_registry reap test that passes in isolation), isort/flake8 clean, tsc clean, vitest 4513 passed. One mypy error in src/kiro_crew/vector_memory.py:844 (faiss write_index overload) — untouched by this branch and environment-dependent; CI's lint gate is authoritative.

New tests this round: atomic-claim, same-branch race sparing the winner, cleanup sparing another branch's worktree, cleanup sparing an unclaimed branch, slug-collision refusal, local-filter refusal (parametrized over process/smudge), unrelated filter.* key still allowed, unresolvable base creating nothing. Screenshots re-pinned to 8e16a445.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 8e16a44 to c0c0d9f Compare July 26, 2026 00:15
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 4 disposition — head c0c0d9f5

Five findings, all fixed. Windows shard 4 went green on the previous push, confirming that failure was a shard artifact.

GPT HIGH — cleanup could delete a directory it did not create (worktree.py)

Correct, and the sharper half of the finding is the conflation: _worktree_branches() returned {} both for "nothing registered" and "the git query failed", and cleanup treated that as authorization to rmtree. Ownership is now proved, not inferred:

  • _worktree_branches() returns None on a failed listing, distinct from an empty mapping. _create_worktree_sync refuses with 503 rather than proceeding blind.
  • The destination is claimed with an atomic os.mkdir(dest) before git runs. EEXIST means someone else owns the path → 409 (and the branch claim is released). A successful mkdir is the only thing that later authorizes deleting that directory — _cleanup_partial(..., created=True).
  • git worktree add accepts an existing empty directory, so pre-creating it costs nothing; its "already exists" refusal applies to non-empty paths (verified against real git).

Cleanup now takes both proofs explicitly: claimed for the branch, created for the directory. Tests cover a created=False call leaving a populated squatter directory untouched, and a _worktree_branches → None call still removing a directory the request created.

GPT HIGH — branch bypassed the redactors (chat_handlers.py)

Accepted. The old comment claimed redaction "would only corrupt a legal branch name", which had it backwards: branch travels further than the text fields — git ref, directory name, SEL records, logs — and AKIAIOSFODNN7EXAMPLE is a perfectly legal FOLLOWUP_BRANCH_RE match.

branch now goes through redact_exfiltration_urls + redact_credentials, and if either changes it the field is dropped rather than shipped mangled; the card then derives a branch from the title. Two tests: credential-shaped branch omitted from the broadcast, ordinary branch passed through intact.

GPT MEDIUM — prefill after a failed session switch

Real: the catch fell through to setPendingInput, so a failed switch put the prompt in whichever session was on screen. It now throws (card retained, message names the worktree path) and there is a second post-switch activeSlotRef.current === slotKey assertion, so the prefill cannot run unless the intended session is actually in focus.

GPT MEDIUM — per-slot follow-up state never pruned

Real. state.followups[slot] survived slot deletion and stale-slot pruning, so multi-KB prompts leaked for the tab's lifetime. Added to deleteSlot.fulfilled and to the sseSlots prune map. Two reducer tests.

GPT MEDIUM — system spec not updated

Fair per the repo's spec-management rule. docs/system-specs/modules/learn-cron-dashboard.md (Key Endpoints) now documents both routes, the MCP tool and its validation limits, the followup_card WS event and delivered semantics, the worktree security boundary (slot-project allow-list, filter refusal, -c overrides), the concurrency contract (atomic branch + mkdir claims, proof-scoped cleanup, 503 on unreadable listing), branch-aware reuse, and the frontend prefill/cleanup behaviour.

docs/followup-suggestions.md is kept as the user-facing guide (registered in docs/index.md, same convention as other feature docs) and is now cross-referenced from the spec, which holds the authoritative contracts.

Gates

pytest 17228 passed (excluding 3 test_dashboard_origin failures that reproduce on clean main on this host), isort/flake8 clean, tsc clean, vitest 4515 passed, eslint 0 errors. Screenshots re-pinned to c0c0d9f5; the card's rendering is unchanged this round, all five findings were in handler logic, state plumbing and docs.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from c0c0d9f to 1dc7876 Compare July 26, 2026 00:52
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 5 disposition — head 1dc78769

HIGH worktree.py:108core.hooksPath=.git/kirocrew-no-hooks is repo-writable. Fixed.
Correct, and the previous rationale was wrong: core.hooksPath resolves relative to the repository, so the sentinel lived under .git/, which whoever prepared the checkout can write. Planting .git/kirocrew-no-hooks/post-checkout turned the suppression into the execution vector.

core.hooksPath now points at an absolute, gateway-owned directory created once per process with tempfile.mkdtemp and narrowed to 0o500, so neither the repository nor a concurrent actor can populate it. The path is cached for the process lifetime, so it stays valid across every git call in a request.

Regression coverage (test/test_worktree_create.py):

  • test_hooks_path_is_not_a_repo_writable_location — plants an executable post-checkout at the old sentinel path and asserts the marker is never created.
  • test_hooks_dir_is_outside_any_repo_and_empty — asserts the configured path is absolute, empty, cached, non-writable, and actually present in the argv.
  • The pre-existing control test (hook does fire unguarded) still passes, so the guard isn't passing vacuously.

MEDIUM ChatPage.tsx:2322activeSlotRef can be stale after switchSlot().unwrap(). Fixed.
Both checks now read store.getState().chat.activeSlot, which reflects the committed reducer state at the moment unwrap() resolves, instead of a ref that only refreshes on render. The false-negative path (error thrown and prefill skipped despite a successful switch) is gone.

Also updated: the module docstring and docs/followup-suggestions.md now state why the hooks directory must live outside the repo, so the trust model matches the code.

Gates: pytest 17229 passed, flake8 + isort clean, tsc clean, vitest 4515 passed (389 files), eslint 0 errors. The 3–4 remaining backend failures are the documented pre-existing host cases (test_dashboard_origin port-env leakage, one pty integration test) and are untouched by this diff.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 1dc7876 to 5931e92 Compare July 26, 2026 01:54
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 6 disposition — head 5931e926

HIGH worktree.py:324 — worktree-scoped config escapes the filter probe. Fixed (confirmed exploitable first)

Reproduced end-to-end on a throwaway repo before changing anything:

git config extensions.worktreeConfig true
git config --worktree filter.evil.smudge "sh -c 'touch $T/PWNED; cat'"
git config --local    --name-only --list | grep -c '^filter\.'   -> 0     # probe blind
git config --worktree --name-only --list | grep  '^filter\.'     -> filter.evil.smudge
git worktree add ... ; test -f $T/PWNED                          -> YES-EXECUTED

So the driver really did run during checkout, and --local really did not see it.

_local_checkout_filter is now _checkout_filter and probes every config scope git reads inside a repository:

  • --local (.git/config) — as before.
  • --worktree ($GIT_COMMON_DIR/config.worktree) — added, gated on _worktree_config_active().
  • A scope that cannot be read at all now returns a refusal sentinel instead of "". Previously a git error was treated as "no filter found", which was the wrong default for a security probe.

_worktree_config_active() requires both extensions.worktreeConfig=true and an existing config.worktree file. That second condition is load-bearing, not defensive padding: with the extension on but no file, git config --worktree --list exits 128 (unable to read config file), so an unconditional probe combined with fail-closed would have 409'd every repo that merely enables the extension. Verified that exit code directly.

Global/system scope is still deliberately not probed — that is the user's own machine (git lfs install), not something the repository supplies.

New tests in test/test_worktree_create.py:

  • test_worktree_scoped_filter_config_is_refused (parametrized over .process / .smudge) — asserts the key is invisible to --local as a precondition, then asserts 409, no destination directory, no branch left behind.
  • test_worktree_config_enabled_but_empty_still_succeeds — the extension alone must not refuse.
  • test_probe_failure_fails_closed — an unreadable scope refuses.

MEDIUM config-baseline.json:584 — unrelated drift. Fixed

Correct, and this was my error twice over: the session.empty_response_auto_continue baseline entry is regenerated locally by running the suite, and a git add -A on an amend pulled it back in after I had already dropped it in round 3. The file is now identical to origin/main in this branch (git diff origin/main...HEAD -- config-baseline.json is empty), and the PR is back to 27 files.

MEDIUM docs/followup-suggestions.md:1 — new markdown file. Disputed, not changed

The rule cited (AGENTS.md:125, "MUST NOT create additional markdown files unless explicitly instructed") sits inside the Specification Management section, whose surrounding clauses are all about docs/system-specs/** — it constrains inventing new spec documents instead of updating the module spec. This PR complies with that section directly: it updates the existing module spec rather than adding one (round 4 added the spec section for this feature).

src/kiro_crew/docs/ is a different, indexed surface — the in-product docs library, one file per feature, reached through docs/index.md. Treating it as closed to new entries would contradict its own structure and recent merged precedent (webex-integration.md in #163, discord-integration.md in #122, feature-tips.md in #103 were all added the same way). Folding a user-facing feature doc into dashboard.md would also bury it: that file documents the dashboard shell, not per-feature MCP tools.

Happy to move it if a maintainer reads the rule as repo-wide — flagging rather than silently complying, since the fix as proposed would make this PR inconsistent with the three merged PRs above.

Gates: pytest 17234 passed (4 new), flake8 + isort clean, mypy 474 files. The 3 remaining backend failures are the documented pre-existing host cases (test_dashboard_origin port-env leakage). No frontend change this round. Screenshot URLs re-pinned to 5931e926.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 5931e92 to 3b138b0 Compare July 26, 2026 14:30
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 7 disposition — head 3b138b0b (rebased onto main)

GPT HIGH worktree.py:326 — linked worktrees keep config.worktree under $GIT_DIR. Fixed (confirmed exploitable)

Reproduced before changing anything, on a linked worktree of a throwaway repo:

git rev-parse --git-common-dir    -> /tmp/…/main-r/.git
git rev-parse --absolute-git-dir  -> /tmp/…/main-r/.git/worktrees/linked
<common>/config.worktree exists?  -> NO
<gitdir>/config.worktree exists?  -> YES        # where the filter actually lives
git config --local --name-only --list | grep -c '^filter\.'  -> 0
git worktree add … ; test -f $T/PWNED2          -> EXECUTED

So the round-6 probe was looking in the wrong directory for exactly the case GPT describes. _worktree_config_active() now resolves rev-parse --absolute-git-dir, which is the common dir for a main worktree and <common>/worktrees/<id> for a linked one, so both are covered by one call.

Regression test test_linked_worktree_scoped_filter_config_is_refused asserts the preconditions (file absent under the common dir, present under $GIT_DIR) and then a 409 with no branch left behind. Revert-verified: swapping --absolute-git-dir back to --git-common-dir fails the test.

GPT MEDIUM worktree.py:435 — prune must precede branch -D. Fixed

Correct. When worktree remove fails and the tree 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 docstring promises hit "branch already exists". _cleanup_partial now prunes first, and retries the delete after a second prune, logging a warning if the branch still survives rather than silently leaving it.

Regression test test_cleanup_deletes_the_branch_after_an_rmtree_fallback patches worktree remove to fail. Revert-verified: with the old ordering it fails on claimed branch survived cleanup.

Claude advisory findings (all non-blocking) — 1–4 and 6 applied, 5 already done

  1. Prototype-pollution guardsetFollowupCard / clearFollowupCard / dismissFollowupItem now return on isUnsafeKey(slot), matching the slice convention. New reducer test covers __proto__/constructor/prototype; it asserts no OWN entry was written (a plain toBeUndefined() passes vacuously for constructor, which resolves through the prototype chain).
  2. Error/items desync — errors are index-keyed and Skip shifts indices, so a useEffect on items clears them on any change. New component test: fail item A, drop A, assert B renders with no alert.
  3. Fail closed on a keyless createSlot — an explicit if (!slotKey) throw after the unwrap. The if (slotKey) wrapper below it is now dead and was removed (its body dedented, no logic change).
  4. Harness pathcapture-followup-card.mjs fixture path is now /home/user/workspace/KiroCrew; no username or internal-style path in the public tree.
  5. config-baseline.json — already dropped in round 6; the branch is 27 files with no baseline hunk.
  6. chmod_safe — the hooks-dir narrowing goes through platform_compat.chmod_safe instead of an inline os.chmod/try, so the shim table stays greppable (and it is a no-op on Windows by construction).

Rebase

The PR had gone CONFLICTING. Rebased onto current main; one conflict, in docs/system-specs/modules/learn-cron-dashboard.md, where main's Pull-request-sources paragraph had been rewritten by an intervening merge while my commit still carried the older text alongside the new follow-up paragraph. Resolved by keeping main's paragraph verbatim and re-adding only the follow-up paragraph, whose trust-model sentence I also refreshed to match the code as it now stands (hooks dir outside the repo, both config scopes probed, unreadable scope refused).

Note for anyone building this branch: main moved the vitest environment to happy-dom, so a stale worktree needs npm install in website/ before the suite will collect.

Gates: pytest 17351 passed, vitest 4556 passed (392 files), flake8 + isort clean, mypy 475 files, tsc clean, eslint 0 errors. The 3 remaining backend failures are the documented pre-existing host cases (test_dashboard_origin port-env leakage). Screenshots re-pinned to 3b138b0b.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch 2 times, most recently from c079c78 to ecdf8cc Compare July 26, 2026 18:39
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 8 disposition — head ecdf8cc1

All five HIGHs and both MEDIUMs addressed. Two of the HIGHs I verified were exploitable before changing anything; the details are below.

HIGH worktree.py:373include.path hid the filter driver from the probe. Fixed

Reproduced on a throwaway repo:

git config --local include.path <hostile.cfg>      # hostile.cfg declares filter.evil.smudge
git config --includes --get filter.evil.smudge     -> resolves
git config --local --name-only --list | grep '^filter\.'  -> nothing (probe blind)
git worktree add …                                 -> filter EXECUTED

--includes defaults OFF for a specific scope query, which is exactly what --local/--worktree are. Both probes now pass --includes. New test test_included_filter_config_is_refused asserts the driver is resolvable by git yet invisible without the flag, then asserts the 409 — revert-verified: dropping --includes fails it.

HIGH worktree.py:124,217 — the hooks sink was still same-uid writable. Fixed by deleting the mechanism

core.hooksPath is now os.devnull. A non-directory device has no hook to find and nowhere to plant one, so the mkdtemp + chmod 0o500 + process-lifetime cache and its lock are all gone (verified git worktree add is happy with it). This is the third shape for this control and the first with no writable window at all: in-repo was repo-writable, the temp dir was same-uid writable between calls. test_hooks_sink_is_a_non_directory_device pins it, and the round-5 plant test still asserts the old in-repo sentinel is inert.

On the PATH-shadowing half of that finding: not changed, and I'd rather say so than pretend. _run_git inherits a scrubbed env, and an attacker who can prepend . to the gateway's PATH already runs code as the gateway user, which makes the git resolution a downstream symptom rather than the hole. If you want a fixed absolute binary anyway, say so and I'll add it — it is a small change, just not one this finding's own threat model reaches.

HIGH chat_handlers.py:1819 / worktree.py:619 — app callers could act on slots they do not own. Fixed

Both endpoints now go through one deny_non_dashboard_caller gate: only the explicit request["app"] == "" dashboard claim passes; an app name is refused, and an ABSENT key (auth middleware never ran) is refused too rather than falling through — the same deny-by-default reasoning api_chat_slots_model already documents. Denials are SEL-audited. Refusing outright rather than filtering to owned slots is deliberate: the worktree allow-list is built from every slot's project, and both surfaces are dashboard-UI features. Four new tests (app caller + absent claim, per endpoint).

HIGH chat_handlers.py:1845 — the card went to every WS client. Fixed

The repo already had an owner-scoped channel (_owner_ws_clients, used to keep provider CI status off app sockets); it just had no typed-broadcast entry point. Added broadcast_ws_owners / ws_owner_client_count and moved the card onto it, so an app socket can no longer receive another user's handoff prompts. delivered now counts that same channel — counting all clients would report delivery to a subscriber that never receives the payload. Tests assert the all-clients broadcast is never used and that delivered is 0 when 7 non-owner clients are connected.

HIGH worktree.py:555 — sync filesystem calls on the event loop. Fixed

_allowed_repo_roots (realpath + stat per slot) and both is_sensitive_path/isdir screens now run via asyncio.to_thread, so a slot project on stalled storage can no longer stall the gateway.

MEDIUM ChatPage.tsx:2219 — unqualified clear could delete a newer card. Fixed

followupAddToSession now clears by the rendered card's ts, matching the worktree action.

MEDIUM ChatPage.tsx:2235 — no orchestration coverage. Fixed, and it found a real bug

New website/src/test/ChatPageFollowup.test.tsx drives the page: worktree-before-session ordering, the scope call, the inline worktree failure with no session created, and the scoping failure that deletes the session it just made while keeping the card.

Writing it surfaced a defect the component tests could not see: the prompt was handed over with setPendingInput alone, whose consuming effect is declared BEFORE the per-slot draft-restore effect. When the slot activation and the prefill land in one React commit, the restore runs last and overwrites the composer with the new slot's empty draft — the prompt disappears. The handoff now also seeds PREFILL_STORAGE_KEY for the target slot (the channel the ?sid/popout paths use and the restore effect consumes), so the restore applies the prompt instead of clearing it. The test asserts the seed names the new slot.

One limit stated plainly: the success case does not assert the final card-clear, because switchSlot(...) does not settle under that page harness (it wants hydration machinery the mocks do not provide) — asserting it would be asserting the harness. That branch is asserted in the "Add to this session" case, and the rendered composer is covered there and in the Playwright capture harness.

Scope

An npm install (main moved vitest to happy-dom) rewrote one engines field in website/package-lock.json. Caught and dropped before pushing — the branch is 29 files with no lockfile or baseline hunk.

Gates: pytest 17358 passed, vitest 4560 passed (393 files), flake8 + isort clean, mypy 475 files, tsc clean, eslint 0 errors. The 3 remaining backend failures are the documented pre-existing host cases (test_dashboard_origin port-env leakage). Screenshots re-pinned to ecdf8cc1.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from ecdf8cc to 669a207 Compare July 27, 2026 02:37
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Jul 27, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 9 disposition — head 669a207a (rebased onto main)

Both HIGHs and all five MEDIUMs fixed. One of them was a functional break of the whole feature, so I want to be explicit about it first.

MEDIUM chat_handlers.py:1821 — the gate 403'd every MCP call. Real, and the most important finding of the round

Verified in the auth middleware: the loopback branch that validates X-Internal-Secret calls the handler and returns without setting request["app"] — there is no app identity to set. My round-8 deny-on-absent gate therefore refused the exact path suggest_followup arrives on, so the tool could never have raised a card. Round 8's own tests missed it because they were a hand-rolled claims stub, not the real middleware.

The middleware now marks that grant (request["internal_auth"] = True) and the gate permits it. A request with neither marker still fails closed — that genuinely means no authentication ran.

Coverage is now at the seam where it broke: TestRealMiddlewareIntegration drives the endpoint through the real auth middleware, asserting a valid internal secret from loopback reaches the handler and a wrong one never does. Revert-verified — removing the marker fails the first test.

HIGH worktree.py:197 — sandbox routing. Fixed; the earlier classification is withdrawn

_run_git now goes through sandboxed_spawn_argv, matching git_coord.py, which already routes agent-influenced git for the same reason. The BENIGN_SPAWNS entry and its justification comment are deleted, and the module docstring section that argued the other way is rewritten.

The one thing I changed relative to the suggested fix is the failure mode's shape: no backend now yields a 503 with a "create the worktree manually" message plus a SEL denied event, rather than an unhandled raise (which would have been a 500). That is still failing closed — the spawn does not happen — and test_sandbox_unavailable_refuses_with_503 pins it, including that no branch is left behind.

Worth stating plainly: this makes worktree creation unavailable on hosts with no sandbox backend (Windows, macOS without sandbox-exec) unless the operator sets agent.sandbox_allow_unsandboxed_exec. That is the trade the blocking rule asks for, and it is now documented in the feature doc rather than worked around.

HIGH ChatPage.tsx:2254 — session activated before scoping finished. Fixed

createSlot gained an activate option (defaults true, so every existing caller is unchanged); the worktree flow passes activate: false, awaits chatSlotProject, and only then switches. The window where the composer was live while the CWD was still pending is gone.

This also closed something I could not assert last round: with activation deferred, the ChatPage test now verifies the prompt actually reaches the new session's composer end-to-end, rather than only that the prefill was seeded. New test does not activate the new session until scoping has completed holds chatSlotProject open and asserts the origin session is still active.

The four remaining MEDIUMs

  • validation.py:570foo..bar, a component ending in . or .lock, and HEAD all satisfied the character grammar (confirmed) and failed only after the branch was claimed, surfacing as a misleading "Branch already exists". is_valid_followup_branch() rejects them at both layers; parametrized tests assert the regex alone still accepts each one, so the tests cannot pass vacuously.
  • FollowUpCard.tsx:66 — an itemsGen counter: a request that rejects after items changed no longer writes its error against the new list's index.
  • state.py:2773ws_owner_client_count() skips closed sockets, so delivered cannot report delivery to a socket awaiting cleanup. Covered in TestOwnerScopedBroadcast alongside the owner-only routing.
  • worktree.py:279worktree list --porcelain -z with NUL-delimited parsing, so a path containing a newline matches its registered entry and a retry reports reused instead of 409. The test creates a real worktree at a path with an embedded newline (POSIX-only; NTFS rejects it).
  • ChatPage.tsx:3660 — Skip now passes the rendered card's ts and dismissFollowupItem ignores a mismatch, mirroring clearFollowupCard.

CI

The three non-review reds were all inherited, and the rebase clears them: Frontend Tests failed on KiroGhostMark.test.tsx, which arrived on main with #446 and was fixed by #522 (assert mask via server render) — my branch had the former and not the latter. Coverage Gate failed only because that job did. Both are green locally on the rebased tree.

Gates: pytest 17453 passed, vitest 4599 passed (395 files), flake8 + isort clean, mypy 475 files, tsc clean, eslint 0 errors. One backend failure under full-suite xdist load (test_dashboard_approval.py::TestRefusalRecovery::test_host_gate_deny_enqueues_recovery_continuation) passes in isolation on this branch AND on a clean origin/main checkout — load-sensitive, not from this diff. Screenshots re-pinned to 669a207a.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 27, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Jul 27, 2026
@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 1d9edfa to bbab33c Compare July 27, 2026 15:07
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Jul 27, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 12 disposition — head bbab33cd

BLOCKING chat_handlers.py:1837 — non-owner dashboard identities passed the gate. Fixed

Legitimate. My round-8 gate checked only that the app claim was the explicit "" dashboard value, which is necessary but not sufficient: a dashboard credential minted for a different subject carries the same shape. It would have raised cards in the owner's composer and, on the sibling endpoint, created worktrees and branches in the owner's repositories.

Both surfaces now require the owner's own identity via is_owner_dashboard_request — the same predicate the source-provider mutations use, rather than a new one. That predicate matters for the standalone-local case too: owner_id is only set when Slack is configured, and the browser's own credential is minted for local-app, so a bare owner-id equality check would have refused the feature on every install without an owner. When no owner is configured only the signed local bootstrap subjects are accepted; an arbitrary subject is refused in that mode as well.

Three regressions on the card endpoint (non-owner subject refused, unconfigured-owner local subject allowed, unsigned subject in that mode refused) plus one on the worktree endpoint proving no branch is left behind. Revert-verified: restoring the round-8 condition fails all three of the refusal tests.

BLOCKING chat_handlers.py:1908 — socket count reported as delivery. Fixed

Also legitimate, and the failure mode is the one this field exists to prevent. delivered was ws_owner_client_count(), taken before any send ran, while the send itself was fire-and-forget — so an owner window that dropped in that gap produced a failed send already reported as delivered, and the model was told the user saw prompts that went nowhere. Nothing is stored server-side to re-deliver.

New DashboardState.deliver_ws_owners() awaits the sends concurrently, absorbs failures per socket (one dead peer must not hide a successful delivery to another window), evicts failed and closed sockets, and returns the number that completed. The endpoint awaits it and reports that count. A delivery exception degrades to delivered: 0 rather than a 500 — the honest answer is "nobody saw it".

broadcast_ws_owners had no remaining production caller after this change, so it is deleted rather than left as an unused public method; its two tests now exercise the awaited path. Revert-verified: making the count len(targets) instead of completed sends fails the new test.

Also in this push: the last Windows shard-4 failure

TestRound9Hardening::test_a_real_git_failure_is_still_a_git_failure patches subprocess.run to assert _run_git passes an ordinary non-zero git exit through, but the sandbox chokepoint raises first on a host with no backend, so the assertion was never reached. Rather than skip it and lose the coverage, it and its sibling now stub sandboxed_spawn_argv with a pass-through — which also fixes a latent problem in the sibling: test_launcher_isolation_failure_is_a_refusal_not_a_git_error was passing on Windows for the wrong reason, since the backend RuntimeError is surfaced as SandboxUnavailable too, so its pytest.raises held without the code under test running.

Gates: pytest 17476 passed, isort + flake8 clean. The 3 remaining failures are the documented pre-existing test_dashboard_origin host-env cases; a 4th (test_apps_registry probe reap) is load-sensitive under -n 8 and passes in isolation. Screenshots re-pinned to bbab33cd.

@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch 2 times, most recently from c6568b5 to 2d45c1c Compare July 27, 2026 15:32
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 27, 2026
Adds a `suggest_followup` MCP tool (kirocrew-core) that lets the agent offer
up to three concrete next steps as a card above the chat composer. Each item
carries an expanded, self-contained 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 the
user presses send, so a single click can never launch an unattended agent turn.

Backend:
- SUGGEST_FOLLOWUP_SCHEMA in validation.py gates item count, per-field types
  and lengths, unknown fields, hidden Unicode, and full-matches an optional
  branch name against FOLLOWUP_BRANCH_RE.
- POST /api/chat/slots/{slot}/followup re-validates the same schema (it is
  reachable over loopback from inside the kiro-cli process group, so it is a
  trust boundary, not a relay), redacts credentials and exfiltration URLs, and
  broadcasts a `followup_card` WS event.
- POST /api/worktree/create builds a sibling worktree via
  sandboxed_spawn_argv + resource_limit_preexec with an argv list and no shell,
  resolves the repo to its git toplevel, refuses sensitive paths, and derives
  the destination server-side.
- The tool is dashboard-only via _resolve_session_key_strict(); Slack, cron and
  subagent contexts fail closed.

Frontend:
- FollowUpCard renders in the same above-composer band as the question card,
  gated on slot ownership, with a single-flight guard and inline error on
  worktree failure.
- Skip drops one suggestion and keeps its siblings; the card clears when its
  last item is gone.

Tests: 49 backend (schema, endpoint, worktree endpoint against a real throwaway
repo) and 11 frontend (component behaviour plus the four new reducers).
@kyleseaman
kyleseaman force-pushed the feat/followup-suggest branch from 2d45c1c to b1d42b9 Compare July 27, 2026 15:56
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 13 disposition — head b1d42b93 (rebased onto main)

First: why no CI ran on bbab33cd

Worth recording because it looked like a flaky dispatch. After the round-12 push, only the pull_request_target workflows (PR Readiness) and CodeQL appeared — CI, Build, and all four review bots were never created, and a re-push with a fresh sha changed nothing. The cause was mergeability: main had moved and the PR was CONFLICTING, so GitHub could not compute a merge ref and does not dispatch pull_request-triggered workflows at all. Rebasing onto main (19 commits) made it MERGEABLE and every workflow dispatched immediately. One conflict, in docs/system-specs/modules/learn-cron-dashboard.md, where main rewrote the Pull-request-sources paragraph; I kept main's paragraph verbatim and re-added only the follow-up paragraph.

BLOCKING worktree.py:515 — cleanup could delete a branch another worktree adopted. Fixed

Legitimate. Compare-and-delete only proves the ref has not moved; it says nothing about who is now standing on it. update-ref -d has none of branch -D's "used by worktree" protection, so a concurrent git worktree add that checked out the branch this request claimed would be left on a dangling ref when the failing request cleaned up.

Cleanup now re-lists worktrees after the prune (the prune is what clears this request's own stale registration) and keeps the branch when any surviving worktree other than its own destination holds it. An unreadable listing keeps the branch too: adoption cannot be ruled out from it, and a retry reporting "branch already exists" is recoverable where a broken worktree is not.

Two regressions — the adoption case (asserting the other worktree still resolves HEAD to the branch) and the unreadable-listing case. Revert-verified: neutering the guard fails both.

BLOCKING ChatPage.tsx:2218 — "Add to this session" destroyed an unsent draft. Fixed

Also legitimate, and the damage is worse than a visual replace: the pending-input path writes the draft through to storage as well, so half-typed text was gone for good. The prompt is now appended below an existing draft (blank-line separated — a handoff prompt is multi-line prose, not a word to concatenate), reading the live composer value from inputRef. An empty composer behaves exactly as before.

Regression test asserts the exact merged value. Revert-verified: restoring the plain setPendingInput(item.prompt) fails it.

Gates on the rebased head: pytest 18115 passed, vitest 4711 passed (408 files), tsc clean, eslint 0 errors / 238 warnings against CI's 1116 ceiling, isort + flake8 clean. The 3 remaining backend failures are the documented pre-existing test_dashboard_origin host-env cases. Screenshots re-pinned to b1d42b93.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Jul 27, 2026
@iamwhatever
iamwhatever merged commit da39b08 into main Jul 27, 2026
48 of 49 checks passed
@iamwhatever
iamwhatever deleted the feat/followup-suggest branch July 27, 2026 16:39
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Jul 27, 2026
iamwhatever pushed a commit that referenced this pull request Jul 27, 2026
…levant (#597)

PR #461 shipped the suggest_followup MCP tool and its follow-up card, but
nothing prompted the agent to reach for it — usage rode entirely on the tool's
own description, and with MCP Tool Search enabled that spec is not in every
turn's context, so the tool would rarely be surfaced.

Inject a situational, dashboard-only reminder into the per-turn interactive
block (the same place the [OPTIONS:] reminder lives). It is gated to
dashboard: / dashboard_ session keys because the tool rejects Slack, cron, and
subagent contexts, and it is framed as optional and turn-end — not per-turn —
so it raises awareness without becoming noise.

Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…irodotdev#461)

Adds a `suggest_followup` MCP tool (kirocrew-core) that lets the agent offer
up to three concrete next steps as a card above the chat composer. Each item
carries an expanded, self-contained 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 the
user presses send, so a single click can never launch an unattended agent turn.

Backend:
- SUGGEST_FOLLOWUP_SCHEMA in validation.py gates item count, per-field types
  and lengths, unknown fields, hidden Unicode, and full-matches an optional
  branch name against FOLLOWUP_BRANCH_RE.
- POST /api/chat/slots/{slot}/followup re-validates the same schema (it is
  reachable over loopback from inside the kiro-cli process group, so it is a
  trust boundary, not a relay), redacts credentials and exfiltration URLs, and
  broadcasts a `followup_card` WS event.
- POST /api/worktree/create builds a sibling worktree via
  sandboxed_spawn_argv + resource_limit_preexec with an argv list and no shell,
  resolves the repo to its git toplevel, refuses sensitive paths, and derives
  the destination server-side.
- The tool is dashboard-only via _resolve_session_key_strict(); Slack, cron and
  subagent contexts fail closed.

Frontend:
- FollowUpCard renders in the same above-composer band as the question card,
  gated on slot ownership, with a single-flight guard and inline error on
  worktree failure.
- Skip drops one suggestion and keeps its siblings; the card clears when its
  last item is gone.

Tests: 49 backend (schema, endpoint, worktree endpoint against a real throwaway
repo) and 11 frontend (component behaviour plus the four new reducers).

Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…levant (kirodotdev#597)

PR kirodotdev#461 shipped the suggest_followup MCP tool and its follow-up card, but
nothing prompted the agent to reach for it — usage rode entirely on the tool's
own description, and with MCP Tool Search enabled that spec is not in every
turn's context, so the tool would rarely be surfaced.

Inject a situational, dashboard-only reminder into the per-turn interactive
block (the same place the [OPTIONS:] reminder lives). It is gated to
dashboard: / dashboard_ session keys because the tool rejects Slack, cron, and
subagent contexts, and it is framed as optional and turn-end — not per-turn —
so it raises awareness without becoming noise.

Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #3139 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #3139: REBASE. Merged PR #461 supplies the git primitives and the allow-list matcher this PR relocates, and four later merged fixes have moved main's copy ahead of the PR's snapshot. The extraction should be redone from current main rather than replayed from the fork's older copy; everything that makes this PR a feature — the service, the two endpoints, the flag, the slot binding, the prompt git line, the whole frontend — is untouched by main. Files: src/kiro_crew/worktree/git_exec.py, src/kiro_crew/worktree/access.py, src/kiro_crew/dashboard/routes/chat.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants