Skip to content

fix(dashboard): per-chat-slot prompt 'This project' resolution (#7345) - #7715

Merged
iamwhatever merged 1 commit into
mainfrom
fix/7345-per-slot-prompt-resolution
Sep 5, 2026
Merged

fix(dashboard): per-chat-slot prompt 'This project' resolution (#7345)#7715
iamwhatever merged 1 commit into
mainfrom
fix/7345-per-slot-prompt-resolution

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Closes #7345.

Blocked on a decision, not on a fix (GPT 5.6 Review)

GPT 5.6 Review is the only red check on 41909a3f; every other lane is green (the cancelled ones are superseded concurrency twins whose newest instance passed). It raises two BLOCKING findings, both anchored in this PR's own new code, so this is neither infra nor flake. Both were checked on merit, and neither is a defect this diff introduces — each is a residual of a pattern the repository already ships, which this PR conforms to and, for the first, strictly narrows. Both match the lane's deterministic unbounded-harm grep (residual/security, authoriz), so they are withheld from Opus adjudication and keep blocking regardless of it; a prose rebuttal cannot clear the gate by design. The analysis is recorded here so the decision is made once, on evidence.

F1 — prompts.py:516, "mutable path confinement permits prompt-root swap." Accurate, and already carried in the code as a KNOWN RESIDUAL block. The window is imposed by the shared primitive, not by this PR: hooks.safe_read_file_bytes_nolink resolves within_root by name at read time (hooks.py:2387), and that line is byte-identical on mainhooks.py is not in this diff. The primitive has ~40 call sites across 31 files, and main's own prompts.py already passes a name-based within_root at five sites carrying the identical residual. This PR narrows the window from a whole re-resolving scan to two adjacent syscalls: the swap must now land between _prompt_dir_entry's resolve and that realpath, and a swap landing on either side of that pair mints nothing. Closing it needs the read to happen relative to a held directory descriptor — a safe_read_file_bytes_nolink contract change shared with its other consumers, including two readers on this same surface, plus a decision about the name-based fallback on Windows, where _DIR_FD_SUPPORTED is False and there is no dir_fd read to fall back to. Narrowing it further without closing it is what the previous round already did, and a post-read re-check of the root only raises the bar to a swap-and-restore straddling the read. So the honest fix is one change that moves every reader, not two call sites here.

F2 — prompts.py:271, "app authorization proceeds when SEL auditing fails." The authorization is the ownership test on the line above the audit (getattr(slot, "_app", "") == request_app); the audit records that grant, it does not confer it. When the audit write fails the app reads its OWN slot's project — a capability it holds independent of the log. The prescribed fix (critical=True, return None) would make this the only one of three sibling app-isolation gates that turns an operator-side storage failure into a capability withdrawal: _deny_non_owner_skill_trust and _deny_foreign_app_skill_slot carry the byte-identical best-effort shape, with the identical # noqa: BLE001 — preserve authorized access if SEL is unwritable comment, both already on main (prompts.py:81 and :133 there) and both untouched by this diff. It also runs against the repository's own stated doctrine for the flag: critical=True is for audit-or-deny, "where a caller must refuse rather than run something unaudited" (name_grant.py:1378) — nothing runs unauthorized here. Applying it to one of the three is an inconsistency; applying it to all three changes documented behaviour on the skills surface and belongs in its own change.

What this needs — a maintainer's call, one of:

  1. /ai-review override gpt 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14: <reason> (repository writer, keyed to the current head), if the analysis above holds; or
  2. land the dir_fd contract change on hooks.safe_read_file_bytes_nolink first — moving every reader and settling the Windows fallback — and rebase this PR onto it, which closes F1 at the root; and separately settle the fail-open/fail-closed policy for app-isolation audits across all three gates together, which settles F2.

Neither is a change this PR should make on its own: the first is a keystone security primitive with a cross-platform behaviour decision attached, the second inverts a policy this same file already applies twice.

One further item wants an explicit yes/no rather than a CI decision: the listing-capability withdrawal in Withdrawn behaviour below. A hand-symlinked individual prompt under ~/.kiro/prompts stops being listed and stops resolving for @mention. That is a deliberate consequence of making listed and serveable one set, and it is the half of #8249's ratchet that costs an existing user something, so it should be approved rather than discovered.

Problem

Prompt local/"This project" scope resolved through _agent._project_dir(), which reads the process-wide KIROCREW_PROJECT_DIR. That variable is auto-detected from the agents/ + skills/ markers, so it names the Kiro Crew source tree on a git install and nothing at all on a wheel install. Either way it is not the user's project.

Every prompt surface read that one variable — the five HTTP handlers and the chat @mention expander. On main, _find_prompt(raw_name) takes no project argument and its lister resolves the local directory from _project_dir(), so a prompt the user authored in their own checkout was invisible to all of them alike: not listed in the Prompts tab, not creatable or editable there, and not matchable by @mention. The two halves did not disagree with each other — they agreed on the same wrong answer. That is why both move here: fixing only the HTTP half would introduce the disagreement, and fixing only the chat half would leave the tab listing the wrong tree.

Scope of "every", stated precisely: every prompt surface, which is what this PR closes. The underlying shape — a per-request "which project?" answered from the process-wide KIROCREW_PROJECT_DIR — has two known siblings outside the prompt surface that this PR deliberately does NOT touch: dashboard/handlers/files.py:2578 (_resolve_project_relative, which resolves request-relative media paths against it and so answers cannot_resolve on every wheel install) and files.py:3127 (the file-search fallback root). They are the same root cause on a different surface, with their own request shapes and their own tests, so they belong in their own change rather than widening this one. handlers/core.py:971 and the updates.py / terminal_commands.py uses are NOT siblings: they locate the install tree itself (changelog, updater, script paths), which is exactly what that variable is for.

Change

One seam, _prompt_local_project(request, state, session_key), answers "which project does this request's local scope name" for all five prompt HTTP handlers (api_prompts, api_prompt_detail, api_prompts_create, _api_user_prompt_detail, _api_prompt_write). Because list, scoped read, create, update and delete all consume the identical resolved value for one request, they cannot disagree: a prompt created there is one the same request lists, and the bytes a scoped read seeds the editor from are the bytes that scope's PUT replaces.

  • Lister (_list_aim_prompts, handlers/__init__.py): split into _build_prompt_base (edition SOPs + ~/.kiro/prompts globals — project-independent, so still cached under the 5s TTL) plus _scan_prompt_dir for the local half, appended fresh on every call and never cached. No cached answer for one project can be served to a caller that resolved a different one.
  • Create/mutation resolver (_user_prompt_dir, its wrapper _resolve_prompt_dir, and the confinement check _local_prompt_dir_in_project, prompts.py): all take the caller-resolved project_dir instead of reaching for the process-wide one, so the symlink-confinement check is made against the very root the write was addressed to.
  • Chat surface (chat_runner.py): @mention expansion and the chat-side prompt listing pass slot.project, so a chat matches its own checkout's prompts.
  • Off the event loop, filesystem half only (chat_runner.py): expansion resolves and reads files under a directory the gateway does not own and that may be network-backed, and the local half is uncacheable, so the cost cannot be amortized away — on the loop, one @mention on slow storage stalls every other request and the heartbeat with it. So the split is drawn by which thread may do the work: _resolve_prompt_mention takes the message and a project directory, is handed neither the slot nor the state, and goes to asyncio.to_thread; the "Loaded prompt" chip it returns is appended by _surface_prompt_chip back on the loop, after the await. Both call sites go through _expand_prompt_mention_off_loop, which does exactly that, and _expand_prompt_mention stays as the synchronous on-loop entry point for callers already on that thread. await keeps the ordering identical to the inline call: nothing after that line runs until the expansion and its chip are complete. Offloading the WHOLE function instead would put slot.append on a worker, and that is not safe: it ends in slot.event.set() on an asyncio.Event, whose waiters are resolved through the loop's call_soon rather than its threadsafe variant, so a foreign-thread append queues a callback the loop is never woken for — and raises outright under asyncio debug mode, failing the turn. The offloaded function's signature is what pins it: a worker cannot reach the slot it is not given.
  • The unscoped detail GET reads in ONE executor job (prompts.py): api_prompt_detail's unscoped branch offloaded _find_prompt and then finished on the loop — hooks.validate_file_path, the size stat and the body read_text. That was survivable while a match could name nothing but a package root or the gateway's own ~/.kiro/prompts; this PR is what lets a match name <project>/.kiro/prompts, so those two syscalls became a stall on storage the gateway does not own. They now share _resolve_and_read, the shape _api_user_prompt_detail's _read already has: the closure returns (entry, content, error_token) and the loop maps the token to the same status, body and SEL outcome each case answered before, so nothing observable changes. _prompt_local_project is the only step left on the loop, and it reads state._slots and nothing else.
  • The dashboard:ui placeholder is not a slot (prompts.py): it is folded to "names no slot" before _named_slot or either resolver sees it. The browser sends that literal whenever it has no chat to name — including this API's create, update and delete, while the listing GET sends no header at all — and the slot-name split turns it into ui, a name a user's own chat can carry. Honouring it pointed every settings-page write at that chat's checkout while the listing answered from the shared project: exactly the create/list disagreement this seam exists to prevent, and a write into a project the request never named.
  • One entry gate (handlers/__init__._prompt_dir_entry): every user-prompt entry, from the directory scan and the exact-name lookup alike, is minted here, so "this name is a plain file inside its own prompt directory" is a property of the entry rather than something each consumer re-establishes. It refuses a linked ENTRY outright with platform_compat.is_link_or_junction — lstat-based, nothing dereferenced to reach the verdict, Windows junctions covered — which is the same predicate the scoped read and both write verbs already apply. Containment stays resolved-to-resolved so a dotfile-managed ~/.kiro keeps working. Every filesystem call is wrapped and RuntimeError is caught alongside OSError/ValueError, because Path.resolve() raises RuntimeError on a symlink loop: a cloned project shipping loop.md -> loop.md loses one entry instead of taking GET /api/prompts down with a 500.
  • One root gate too (prompts._resolve_prompt_dir, for the LOCAL scope): the entry gate compares an entry against the directory it was found in, which by construction cannot see whether that directory belongs to the project — when the root itself is a link, both sides resolve into its destination and every path inside looks confined, exactly as _linked_prompt_root documents. So the local scope's two enumerating callers (_list_aim_prompts and _local_prompt_entry) resolve the root through the same helper the scoped read and both write verbs use, and treat a linked_prompt_root answer as an empty local library. Without it, a cloned repository shipping .kiro/prompts -> ~/Documents (or .kiro -> ~) would have the filename and first-heading description of every *.md under the directory it named published by GET /api/prompts, and @<stem> would inject up to MAX_PROMPT_BYTES of one — while every serving verb answered linked_prompt_root for the same name. is_sensitive_path does not cover this: it is applied to the resolved entry, and an ordinary home-directory document is not sensitive. This is the sibling of the entry-link refusal above, and the two together are what make "listed" and "serveable" one set for the local scope. That gate catches a symlink LOOP too — RuntimeError, not an OSError — and there the loop needs no race to arrive: _linked_prompt_root asks os.path.islink, which swallows the ELOOP and answers False, so a checkout shipping a cyclic .kiro reaches _local_prompt_dir_in_project's resolve undetected, and that gate runs outside any broad catch on both enumerating callers. Letting it escape was a 500 on GET /api/prompts and on the unscoped detail lookup rather than one empty local library; a loop names no directory inside the project, so it is refused like any other escaping chain. The global root is deliberately left asymmetric (the global scoped read refuses a symlinked ~/.kiro/prompts while the listing still walks it): that directory is a location the operator chose rather than one a checkout can name, and refusing it would withdraw the whole global library from anyone who stows it.
  • Canonicalized @mention read (chat_runner._expand_prompt_mention): the read is performed on the path hooks.validate_file_path canonicalized, and the sensitive-path check is re-applied there, rather than checking the name as addressed and reading what that name resolves to. For a link those are different files, so a check on the name and a read of the target could name different files. Every entry reaching here is minted by the gate above, which refuses a link outright, so this closes a swap landing between that gate and the read rather than a standing hole; package SOP paths arrive already resolved, so it is a no-op for them. Canonicalizing and then opening that name still leaves the swap window open, so the read itself now goes through hooks.safe_read_file_bytes_nolink — the same gate the scoped read uses: it opens FIRST with O_NOFOLLOW and validates the descriptor it actually read, so the inode checked is the inode injected into the turn, and a hardlinked or non-regular one is refused (st_nlink > 1 is the only signal a second name for a sensitive inode leaves, and it is readable only on the descriptor). within_root is passed only when the entry positively names the local or global user scope, whose root the listing gate already validated it against — that pins the opened inode inside the prompt tree, so an ancestor directory swapped for a link cannot redirect the read out of it; a package SOP's roots are plural and come from the platform seam, so it keeps the containment its own provider gave it rather than one this path guessed. A refusal reports the same not_found an unreadable file already produced, so it reveals nothing a link's target could be probed with.
  • One read gate for EVERY reader of an entry (prompts.py, handlers/__init__.py): the minting gate refuses a link and a hardlink by lstat, so what a by-name read still lost was the window BETWEEN that lstat and the open — the path is re-resolved there, so an entry swapped in between serves its target's bytes. Two readers were still by-name: the unscoped detail read (path.read_text, a whole file) and the listing's own description read (_extract_sop_description, one heading or description: value). Both now go through hooks.safe_read_file_bytes_nolink, joining the @mention expansion and the scoped read, so the set of readers is closed. A user-prompt entry describes itself through _gated_sop_description; _extract_sop_description is left for the package SOP walk, whose roots the platform seam supplies rather than a checkout. The root each read is pinned inside comes from ONE shared derivation, _prompt_read_root — a root derived per reader is how two readers of one directory drift apart — which answers a root only for an entry positively naming local or global, and None for a package SOP or an unfamiliar entry shape, keeping O_NOFOLLOW and the hardlink check without inventing a containment. A refused or unreadable description yields an EMPTY description rather than dropping the entry, preserving the "a bad mode surfaces as the read path's error, not as a prompt vanishing" contract. The unscoped read's refusals collapse onto the file not readable 500 it already answered for an unreadable file: the gate reads and refuses through one descriptor, so it cannot say which of a link, a hardlink, an escaping inode or a bad mode happened — deliberately, since separating them would make the endpoint an oracle for a link's target — while the sensitive-path refusal is checked before the read and keeps its own access denied 403.
  • Bounded local lookup (_local_prompt_entry): the candidate is taken from the directory's own entry name (os.scandir, one getdents, no per-entry stat) rather than by joining the caller's string onto the prompt root. The two spellings name the same inode — _plain_stem_ok runs first and rejects a separator, a .. and a dotfile — but only the enumerated form makes that unconditional rather than a property of the predicate. The per-turn cost is then O(1) in the number of prompts the directory holds rather than linear in it: the root gate's lstat and two resolves, one getdents, the entry gate's own stats, and — on a HIT only — one description read of the matched file, which the caller then reads again for the body it substitutes. So a resolved @mention opens that one file twice and a miss opens nothing. Stated honestly as a delta: against main's cold or expired 5s cache this is strictly less work, and against a warm cache it is a constant-factor increase (main did zero filesystem work there) — the warm-cache path is not reachable while being correct, because a cached local answer for one project served to another is Per-chat-slot project resolution for the prompt 'This project' scope #7345. Being off the loop bounds who a slow read hurts; it does not make the read free, so both properties are needed and both are pinned.

Rebase onto main (this round) — what merged, and what it cost

The branch was 160 commits behind. Three upstream changes overlap it and each is
reconciled rather than overwritten:

Two Windows-only test splits, also from the rebase. hooks.validate_file_path gained
a Windows-only linked-ancestor screen in c041e8d (#6305) after this branch was cut:
on Windows it refuses any path with a symlinked or junctioned ancestor outright, because a
junction whose target is a UNC share turns the realpath below it into the outbound SMB
probe its lexical UNC gates exist to prevent, and Windows has no O_NOFOLLOW to fall back
on. POSIX takes the opposite trade — an unconditional walk there would refuse a symlinked
/home. Two tests were asserting a platform-neutral outcome that is no longer one:

  • test_a_kiro_link_that_stays_in_the_project_still_lists_and_resolves split into three.
    The root gate's tolerance (a .kiro link that stays inside the project still LISTS) is
    platform-neutral and keeps its exact assertion; the mention RESOLVING it is now POSIX-only,
    and the Windows outcome is pinned positively as the blocked that screen produces, with
    first_linked_ancestor and validate_file_path asserted directly so the refusal is
    attributed to that gate and cannot be mistaken for a root-gate regression.
  • the new test_the_unscoped_read_is_not_redirected_by_a_root_swapped_after_the_mint
    asserts the property ("the swapped root's file is not served") on both platforms and the
    refusing stage per platform: 500 on POSIX (the read root refuses), 403 on Windows (the
    ancestor screen refuses one gate earlier). Nothing is skipped.

Six of #8249's tests changed their assertion, and that is the one thing worth a
maintainer's eye.
They were written against a listing with no entry gate, so they pin
"the entry stays listed, only its metadata or its bytes are withheld". This PR adds
_prompt_dir_entry, which refuses a linked or hardlinked entry outright — the
withdrawal already named under Withdrawn behaviour below, and the reason is agreement:
the local scoped read and both write verbs refuse those entries, so a listing that kept
them advertised names no verb on this API will open. Five of the six now assert something
strictly stronger (the name is not offered at all, and the planted bytes still never
appear); the audit-line test is driven by an unreadable mode instead of a hardlink,
because a mode-denied plain file is what still reaches the description reader and is the
shape the "listed with no description" contract exists for; the blocked audit outcome
moves to a package SOP, the reader that still canonicalizes a name it was handed. The
sixth, test_a_prompt_symlinked_to_an_ordinary_file_still_describes, was explicitly a
ratchet against tightening the description read into a blanket link refusal. That
ratchet is preserved — it now asserts _gated_sop_description still describes a link
whose target stays inside the pinned root — but the listing half of it is genuinely
withdrawn, and no rewording changes that. If the withdrawal is not wanted, the thing to
drop is the entry-level link refusal, not the test.

Also fixed this round, from the GPT 5.6 Review: the prompt ROOT is pinned where it is
validated, in two places, because the first pass only covered the first of them.

  • The mint (prompts._local_prompt_scan_root): the root is resolved once by the gate
    that approved it, and _prompt_dir_entry / _scan_prompt_dir / _gated_sop_description
    compare against that fixed value instead of re-resolving the caller-addressed name. A
    root swapped for a link after validation yields an empty local library rather than
    publishing the directory the swap named.
  • The readers (prompts._prompt_read_within_root over _prompt_read_root): both
    readers were still assembling a fresh, unresolved <project>/.kiro/prompts as their
    within_root, and hooks.safe_read_file_bytes_nolink realpaths what it is handed — so
    the chain was re-traversed at READ time and an outside file carrying the matched prompt's
    own name passed the fd-real-path check. Reproduced before fixing: the unscoped read
    returned "content": "# OUTSIDE-ROOT-HEADING\n[REDACTED: credential]\n" and the
    @mention resolved ok. A user scope now gets its root resolved through that scope's
    own gate, and None there means REFUSE rather than "unconstrained" — a fallback to the
    canonical path's own parent in that state would pin the read inside exactly the directory
    the swap named. A package SOP keeps that fallback, which is the only root available
    across plural seam-supplied roots and the one thing that carries the guarantee onto
    Windows.

Known residual, recorded in code, in the spec and in the tests rather than implied.
within_root is a PATH and hooks.safe_read_file_bytes_nolink realpaths it at read
time, so a root swapped between the derivation and that realpath is still followed —
two adjacent syscalls rather than a whole scan, but not closed. Closing it needs the read
performed relative to a held directory descriptor (pinned_fs.open_dir_pinned, which the
write verbs already use), which is a safe_read_file_bytes_nolink contract change
shared with its other ~40 consumers, including the scoped read (prompts.py line 1185 on
main) and the package-SOP description read (#8249, line 289) on this same surface, which
carry the identical residual. Plus a decision about the name-based fallback on Windows,
where _DIR_FD_SUPPORTED is already False. That belongs in one change that moves every
reader, not in three call sites inside a prompt-scoping fix. The GPT 5.6 Review blocks
on this and it is right to
; the maintainer call is between accepting the residual with
an override and holding this PR for that follow-up. It is stated at length in the review
response comments below and I have not requested an override.

Withdrawn behaviour (name it, don't discover it)

The safety envelope above is derived — this PR is what makes an untrusted, cloned <project>/.kiro/prompts reachable in the first place — but it is applied to both user scopes, so it costs existing users two things that were working. Neither is a bug being fixed; both are decisions:

  • A hand-symlinked individual prompt under ~/.kiro/prompts no longer appears. With ~/.kiro/prompts/{plain.md, stowed.md -> ~/dotfiles/stowed.md, alias.md -> plain.md}, main lists alias, plain, stowed and this branch lists only plain. So a GNU Stow / chezmoi per-file link and an intra-directory alias both vanish from the Prompts tab and stop resolving for @mention — on main @stowed expanded, because only the write verbs refused links. What is not affected is the shape a dotfile manager actually produces: a symlinked ~/.kiro, a symlinked project root, and a .kiro linked to another directory inside the same project are all ancestor links, and all keep working (pinned).
  • A redirected prompt ROOT now yields an empty local library rather than a published one (the root gate above). No capability is lost that was usable — every serving verb already answered linked_prompt_root for those names — but the Prompts tab stops showing entries it previously showed.

The migration is the same in both cases and needs no flag: copy the file in place of the link, or point .kiro/prompts at a real directory. Refusing a link is the only way to make the listing offer exactly the names the read, update and delete verbs can address, and the alternative — keeping the entry — is a name the API advertises and then refuses to open.

Design decisions

Which question to ask depends on whether the request names a real chat, and the two cases genuinely differ:

  • A request whose X-Session-Key names an existing slot speaks for one chat, so it resolves strictly per-slot (requesting_slot_project) — the same answer chat_runner reaches from slot.project. A chat with no project of its own is told so rather than shown a neighbouring chat's checkout, which would list a prompt that chat cannot expand and send a local write into someone else's tree.
  • A request that names no slot is a global surface and gets active_project_dir's single-shared-project fallback. This is load-bearing: the only surfaces that offer this scope — the overview Prompts tab and the command palette — sit outside any chat and send either no slot key or the shared dashboard:ui placeholder, which is folded to the same slotless answer, so a strict-only resolver would answer None for every request they can make and "This project" would be permanently dead there. Two chats on different projects still resolve None; a settings page has no defensible answer then, and guessing would create, overwrite or delete in the wrong checkout. That flows into the existing no_active_project contract — no new error codes.
  • A chat genuinely named ui resolves as the slotless surface too. Its real key is dashboard:<slot>, so it is byte-identical to the placeholder on the wire and the two cannot be told apart. Folding is the fail-safe side of that: the shared-project step still answers for the ordinary single-project install, and nothing is written to a checkout the request did not name. Honouring it is the opposite trade — it would hand a chat's project to every settings-page mutation on the install.

App tokens get neither the fallback nor a foreign slot. An app token may READ /api/prompts, and grants are path-only, so honouring an arbitrary X-Session-Key would turn the endpoint into a content oracle for another slot's checkout (prompt names, descriptions, and through the detail lookup, bodies). An app resolves only for a slot it owns; every other case narrows to no local project, which keeps the package SOPs and global prompts its grant does cover. Narrowing rather than 404-ing is deliberate — the rest of the response is not slot-scoped, so refusing would withdraw a capability the grant covers. Both outcomes of that selection are SEL-audited under app_isolation with operation=prompt_local_project — an allowed event names the slot an app was actually served, a denied one names the slot it was refused — so an app request leaves exactly one attributable line either way, and a compromised app's whole reach is reconstructible from grants rather than inferred from an absence of refusals.

Tests

test/test_prompts.py, each pinning one branch (all verified to fail when their branch is reverted):

  • create/list agreement for one slot, and no cross-slot leak (the Per-chat-slot project resolution for the prompt 'This project' scope #7345 bug)

  • X-Session-Key selects one slot's project across a two-slot state

  • a slotless dashboard request reaches the single shared project, for both the empty and dashboard:ui keys, and a create from it lands there

  • a slotless request with two slots on different projects refuses no_active_project and writes nothing

  • a request naming a real slot that has no project gets no neighbour's project, for both list and create

  • TestAppTokenLocalPromptIsolation: an app cannot list or resolve-by-name a foreign slot's local prompts, gets no shared-project fallback (with a dashboard control proving the branch is reachable), still reads its own slot's, and the refusal is audited and survives an unwritable SEL

  • a contained symlink alias is refused, asserted as the agreement with the scoped read's own 403 rather than on its own, and the alias's target is still a prompt

  • a self-referential loop.md drops one entry and not the library: the listing keeps its siblings, GET /api/prompts still answers 200, and the @mention lookup is a miss

  • the @mention read refuses a hardlinked prompt (a second name for an outside inode, which no symlink check can see and canonicalization does not change) and an inode resolving outside the scope root — the two properties only the descriptor-validating gate can enforce; both fail when the read is reverted to reading the canonical name

  • the candidate comes from the directory's entry name: a plain name with no matching entry reaches the entry gate zero times, and a present one reaches it exactly once

  • resolving an @mention in a directory of 13 prompts extracts exactly one description, so the per-turn work does not grow with directory size

  • TestPromptExpansionStaysOffTheEventLoop, which pins the split in both directions: the resolve reports a thread id that is not the loop's while the chip append reports only the loop's (asyncio.run drives the loop on the calling thread, so the two are distinguishable); the offloaded resolver's parameter list carries neither the slot nor the state, because a convention the next caller can forget is not a guarantee and a signature is; and — statically, over chat_runner's own source, because the @mention site cannot be driven without a live session — no coroutine resolves a mention with a bare call. The append half fails when the offload is widened back to the whole function

  • TestEveryPromptReaderUsesTheNoLinkGate, which also carries the table of readers and what pins each: the unscoped detail read refuses a hardlinked prompt and an inode outside the scope root (against the by-name version both answered 200 with the outside file's body — the leak, reproduced); the listing describes a user-prompt entry with _extract_sop_description made to raise, so the entry cannot silently route back through the by-name reader nor pass by having stopped reading; _gated_sop_description refuses a symlink (under requires_symlinks), a hardlink and an outside inode, and still describes a plain contained prompt so the refusals are not the only outcome; and _prompt_read_root is pinned per entry shape, including that an unfamiliar source falls back to no root rather than a wrong one

  • the bounded-lookup description counter (test_resolving_a_mention_reads_only_the_matched_prompt) now counts BOTH description readers, so moving the read back to the by-name variant cannot make its "exactly one, for the matched prompt" bound look satisfied by having stopped observing it

  • TestUnscopedDetailReadStaysOffTheEventLoop: hooks.validate_file_path — the first step after the match, with the stat and read_text behind it in the same straight line — reports the resolution's own worker thread and not the loop's, so one job did all of it; plus a static pass over api_prompt_detail's coroutine body (nested defs pruned, since those ARE the executor jobs) asserting no filesystem method call is left on the loop. Both fail against the split-job version, the second naming ['read_text', 'stat']

  • the three /prompts get status stubs in test_chat_runner_coverage.py and the re-entry identity test in test_active_turn_session_key.py now stub _resolve_prompt_mention — the filesystem half — instead of the on-loop wrapper the command no longer calls, so the real coroutine and the real on-loop chip append stay in the path each one measures

  • TestTheDashboardPlaceholderKeyNamesNoChat: with a slot named ui and a neighbour on a different project, the placeholder key lists none of that slot's local prompts, a local create refuses no_active_project and writes nothing, and a local delete leaves that slot's prompt on disk; with one shared project the placeholder still resolves it, so the tests pin the fold rather than a dead branch

  • a redirected prompt ROOT publishes nothing, in both shapes (.kiro/prompts -> <outside> and .kiro -> <outside>), asserted as the agreement: the listing has no local entries, the @mention misses and leaks no body, and the scoped read's own linked_prompt_root 403 is pinned alongside

  • a .kiro link that stays inside the project still lists and still resolves, so the root gate is containment rather than a ban on links

  • an ANCESTOR symlink loop (.kiro as a two-link directory cycle) costs the local library and not the request: GET /api/prompts answers 200 with the global half intact and no local entries, the unscoped detail lookup is a 404, the @mention is a miss, and the gate's own linked_prompt_root answer is pinned platform-neutrally by raising the loop directly. The cycle is planted with real symlinks under conftest.requires_symlinks: _winapi.CreateJunction checks GetFileAttributesW on its target before creating the link, so the target must already resolve and no junction can close a cycle — the junction fixture failed on windows-latest with WinError 2 instead of planting one. CreateSymbolicLinkW has no such requirement, and the marker PROBES for the privilege rather than guessing the platform, so a runner that holds it (GitHub's Windows runners do) exercises the same defect: on 3.12 Path.resolve() maps ERROR_CANT_RESOLVE_FILENAME to the same RuntimeError POSIX raises for ELOOP

  • the chat and HTTP surfaces resolve one slot identically: Path(slot.project) (chat_runner's own expression) equals _prompt_local_project's answer for the same key, and the consequence is pinned too — each slot's prompt is resolvable and listed for that slot and absent from the other

  • the unreadable-prompt guarantee is pinned platform-neutrally with undecodable bytes (entry kept, empty description); the mode-denied variant is IS_POSIX-guarded, because Windows grants the owner a read regardless of the mode bits

Existing symlink-confinement tests inject the slot project rather than patching the process resolver.

Spec

docs/system-specs/modules/learn-cron-dashboard.md — the Prompts (CRUD) paragraph documents how <project> is resolved, the slot-named vs slotless split, the app-token narrowing, and the single entry gate: the unconditional link refusal and the predicate it shares with the read and write verbs, the resolved-to-resolved containment that keeps an ancestor link working, why that entry-level check says nothing about the prompt ROOT and which two callers therefore gate the root, the deliberate global-root asymmetry, the symlink-loop RuntimeError, and the enumerated candidate behind the bounded local lookup with its honest cost. Updated for that dashboard:ui counts as naming no slot (and why a chat named ui resolves the same way), and — this round — for where the loop boundary actually falls on both halves of the surface: that expansion offloads _resolve_prompt_mention while _surface_prompt_chip appends back on the loop (with the asyncio.Event reason it must), and that the unscoped detail GET does all of its filesystem work in one _resolve_and_read job. It also now records that the set of readers going through safe_read_file_bytes_nolink is closed — the two by-name holdouts, the unscoped read and the listing's description read, included — the single _prompt_read_root derivation behind each read's within_root, and why the unscoped read's refusals collapse onto one uninformative status.

Verification

isort, flake8, mypy --platform linux, check_black_formatting.py, check_subprocess_encoding.py, check_sync_io_in_async.py, check_changelog_history.py, docs-lint.sh, check_harness_parity.py, check_brand_name.py — all exit 0. test_prompts.py, test_chat_slot_project.py, test_security_posture.py, test_chat_runner_coverage.py, test_active_turn_session_key.py, test_skill_browser.py, plus the full backend suite. Each new branch verified to fail when its own branch is reverted — including this round's TestARootSwappedAfterValidationPublishesNothing, which against the re-resolving version reports status == 'ok' with the planted body resolved, i.e. the leak reproduced before it was closed.

Rebased again onto 159a9fb (main cut 0.6.0 mid-round; the changelog-history gate is what caught the stale base). Local: De-Amazon Scrub Lint reproduces on origin/main itself, independently of this diff — CI's own run of that lane is GREEN, so it is a local locale artifact rather than a repo problem, recorded only so the discrepancy is not re-investigated: bash scripts/scrub-lint.sh --no-history on a pristine f59b865bb worktree fails with test/test_atomic_write_named_duplicates.py:155:/home/tést and :157, added by ae45732 (#7164) and in a file this PR does not touch. The gate is not diff-scoped, so it reports the same two lines here.

The five open CodeQL inline alerts on this diff are false positives, and the reason is the same for all of them: the flagged line IS the sanitizer. "Uncontrolled data used in path expression" points at _prompt_dir_entry's own path.resolve() and is_sensitive_path(str(resolved)) — the containment gate that decides whether the entry may be named at all — and at chat_runner's prompt read, whose path hooks.validate_file_path has already canonicalized and refused on — and which now reads through hooks.safe_read_file_bytes_nolink rather than by name. The CodeQL and Analyze (python) checks are both green.

No screenshot: this is backend-only and renders no new UI. The user-visible effect is that the Prompts tab's existing "This project" scope resolves to the user's checkout instead of to nothing — same pixels, correct contents.

Pattern harvest

Rule candidate: review-prompt

Pattern: a prompt-time phrase resolved against process-global state ("This
project") when the surrounding feature is per-chat-slot. The resolution reads
whatever the last slot happened to set, so two concurrent slots resolve the same
words to different projects and neither is wrong locally. Review prompt: for any
value a prompt interpolates, ask which scope owns it, and whether two live slots
could disagree.

Rule candidate: no-blocking-call-on-event-loop

Pattern: a per-request "which project?" resolver is made request-scoped, and the
new answer's directory is one the process does not own. Every cost argument the
old code could make — a process-wide cache, a single well-known root — is
withdrawn by the same change that makes it correct, so the read becomes both
uncacheable and attacker-influenced in latency. Review prompt: when a lookup
stops being process-global, re-ask whether its caller may still hold the event
loop; the answer flips on the same commit that flips the scope.

Rule candidate: offload-the-work-not-the-function

Pattern: a blocking-call finding is fixed by wrapping the whole enclosing
function in asyncio.to_thread, which is coarser than the defect. The function
almost always also touches loop-owned state — an asyncio.Event, a queue, an SSE
writer, cached state the loop mutates — and that state is now mutated from a
worker. It fails silently (a call_soon the loop is never woken for) or only
under asyncio debug mode, so the fix reads as green. Review prompt: offload the
PURE half and hand its result back; make the offloaded callable's signature
unable to reach the loop-owned objects, and let the caller apply the result after
the await.

Rule candidate: placeholder-key-is-not-an-identity

Pattern: a client sends a fixed sentinel in an identity header for "no identity
here" (X-Session-Key: dashboard:ui), and a server-side parser derives a real
identity from it by string surgery — the slot-name split turns the sentinel into
ui, which is a name a user can pick. The sentinel then selects a real
principal on exactly the installs where the name collides, so it is invisible in
testing and never fails loudly. Review prompt: for any sentinel value in an
identity field, check every parser that transforms rather than compares it, and
require the fold to "absent" before the transform, not after.

@bolichen97
bolichen97 requested a review from a team as a code owner September 1, 2026 20:09
@bolichen97
bolichen97 requested a review from buluoray September 1, 2026 20:09
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound per-requester resolution behind one seam, but the fix rides in a silent, unaudited behavior regression for symlinked global prompts.

Watch

  • _prompt_dir_entry now unconditionally refuses a symlinked entry, and the refusal is a bare None with no SEL line — unlike the withheld-description reads, which are audited precisely because "absent" is otherwise indistinguishable from "never existed". A user who symlinks prompt files into ~/.kiro/prompts (a dotfile-manager shape the docstring itself distinguishes from ancestor links) loses listing and @mention for those prompts with zero operator signal; the docstring records the cost ("a hand-symlinked individual prompt … no longer appearing") but nothing records it at runtime. Audit the dropped entry, or at least surface the change at release time.
  • "Which project does this request name" now has two per-surface implementations — skills' workspace scoping and prompts' _prompt_local_project — with deliberately different app-token and dashboard:ui handling, and _DASHBOARD_SURFACE_KEY is now spelled independently in six handler modules (_shared.py, artifacts.py, cron.py, mobile_connect.py, now prompts.py). Each copy is individually justified; collectively they are how the surfaces drift on the same question.

Suggestions

  • Hoist the surface-key literal and the slot/slotless/app resolution shape into _shared as a follow-up, so the skills and prompts answers cannot diverge.

[DESIGN-REVIEWED] 41909a3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The sole candidate — _gated_sop_description catching only UnicodeDecodeError and being called outside _prompt_dir_entry's try — does not survive falsification. _gated_sop_description handles its two real outcomes: safe_read_file_bytes_nolink returns None (guarded) and UnicodeDecodeError (caught). The only unguarded raise inside safe_read_file_bytes_nolink is validate_file_path; every other path (os.open, fstat, read) is wrapped in try/except OSError → return None, and allow_truncate=True suppresses FileTooLargeError. validate_file_path on POSIX resolves via os.path.realpath (default strict=False, does not raise on missing files or ELOOP) and is_sensitive_path, neither of which raises for these inputs. Its only caller, _prompt_dir_entry (__init__.py:651), has already run path.resolve(), path.is_file(), path.stat() and is_sensitive_path(resolved) inside its own guarded try on the same path before reaching the _gated_sop_description call — so any deterministic filesystem failure would already have returned None there and never reached the description read. A raise therefore requires both a TOCTOU race and a platform where realpath raises, which is "could/might," not a concrete reachable outcome. The asymmetry with _extract_sop_description's OSError catch is justified: that sibling calls validate_file_path as its first, unguarded filesystem touch on a package-SOP path, while _gated_sop_description runs only after _prompt_dir_entry's guarded validation. Fails bar (a) and (c); below 80.

[OPUS-REVIEWED] 41909a3

Verdict parsed from the review's SHA-scoped output markers for commit 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks are done: consumers counted, existing mechanisms verified (requesting_slot_project/active_project_dir in _shared.py, the pre-existing social_share.DASHBOARD_SURFACE_KEY), the declared unfixed siblings confirmed at files.py:2627/3175, and the zero-consumer surfaces identified. Here is the review.

First-Principles-Verdict: CONCERNS

The fix is cause-level and honest, but it ships a third named copy of dashboard:ui and two zero-consumer surfaces.

What this change ships

Intent: make "This project" prompts resolve to the requesting chat's own checkout instead of the process-wide install-tree variable (#7345) — a FIX.

  1. Prompt list/read/create/update/delete resolve "local" from the requester — justified (the fix, cause-level)
  2. Chat @mention and /prompts match the chat's own checkout — justified (declared half of the fix)
  3. Slotless Prompts tab/palette use the single shared project; two projects open → fail closed — justified, declared
  4. A chat named ui can no longer capture settings-page writes — declared; constant duplicates social_share.py:46
  5. App tokens see only their own slot's local prompts, each selection audited — rides along, derived (app boundary the fix creates)
  6. Linked/hardlinked/sensitive/odd-stem entries vanish from the listing — rides along, derived (cloned content now reachable)
  7. A repo-redirected or swapped prompt root lists nothing — rides along, derived
  8. Unscoped read of a hardlinked prompt now 404s (was 500) — rides along, declared
  9. Mention expansion and unscoped read move off the event loop — rides along, derived
  10. Local listing half is never cached — declared, derived

Watch

  • _expand_prompt_mention (chat_runner.py:3516) has ZERO production consumers: both call sites use _expand_prompt_mention_off_loop, and chat.py:120 is a noqa: F401 re-export nothing imports. Its "callers already on that thread" docstring names callers that do not exist.
  • _DASHBOARD_SURFACE_KEY (prompts.py:81) is the third named spelling of one wire literal — grep "dashboard:ui" in src: named constants at social_share.py:46 and mobile_connect.py:44, plus 7 raw comparisons (cron, token_auth ×2, _shared ×2, artifacts ×2). The root cause (no owning module for the literal) stays unfixed with ~9 siblings.

Subtractions

  • Drop _DASHBOARD_SURFACE_KEY (prompts.py:81); import the existing DASHBOARD_SURFACE_KEY from social_share.py:46.
  • Delete _expand_prompt_mention and the chat.py:120 re-export (0 production consumers); tests can compose _resolve_prompt_mention + _surface_prompt_chip or call the off-loop variant.
  • Drop _prompt_read_root and _resolve_prompt_dir from the handlers/__init__.py import block (lines 325, 328) — 0 consumers of those re-exports; chat_runner imports only _prompt_read_within_root, and tests address _prompts_mod directly.

[FIRST-PRINCIPLES-REVIEWED] 41909a3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

@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 Sep 1, 2026
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 53cae2e to 09305b1 Compare September 2, 2026 05:25
@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 Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Every HTTP prompt surface now resolves project_dir = None, regressing local scope to a hard failure.

Disposition: fixed — the concern holds exactly as written. The prescribed remedy is rejected and the lane's own Suggestion adopted in its place.

The handlers switch to requesting_slot_project(state, session_key) ("THIS slot, no cross-slot fallback"), but the shipped client sends only the placeholder (_sk = { 'X-Session-Key': 'dashboard:ui' }) or no header
"dashboard:ui" -> slot "ui" -> None. Consequence: local prompts vanish from the Prompts tab and command palette, and every local-scope create/read/edit/delete returns no_active_project

Verified against the shipped frontend rather than taken on trust: prompts() and promptDetail() are bare fetch calls that set no header, and createPrompt/updatePrompt/deletePrompt inherit the _sk placeholder that client.ts itself documents as naming no actual session. The HTTP half therefore had zero callers able to exercise it, and local scope went from wrong-project to permanently no_active_project. A reachable regression of a working capability, so it is in scope irrespective of proportionality.

Fix. All five handlers resolve through one seam, _prompt_local_project(request, state, session_key), which splits the question by caller. A request whose X-Session-Key names an existing slot resolves strictly per-slot. A request naming no slot is a global surface and gets active_project_dir's single-shared-project fallback. That fallback is what makes the HTTP half reachable at all: the only surfaces that offer this scope sit outside any chat and send no slot key.

Rejected remedy, and why.

Fix: land the frontend half in this PR (sessionKey params on the prompt client methods, real dashboard:<slot> key from callers - the exact pattern the skills API already uses), and define what slot the overview-level Prompts tab and palette name, since they live outside any chat.

There is no correct slot for those two surfaces to name, and the trailing clause is the reason. Plumbing a sessionKey through them would require inventing a slot identity the backend would then have to honour, which is a fiction rather than a fix. The skills precedent works because SkillPickerMenu is rendered inside a chat and has a real key to pass; the overview Prompts tab and the command palette are not. The adopted Suggestion reaches the same user-visible outcome with no invented identity.

Pinned in test/test_prompts.py: a slotless request reaches the single shared project for both the empty key and dashboard:ui and a create from it lands there; a slotless request with two slots on different projects refuses no_active_project and writes nothing.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Suggestion: If the management UI is intentionally slot-less, resolve its HTTP surface via active_project_dir (single-shared-project fallback) and reserve strict requesting_slot_project for slot-keyed requests

Disposition: fixed — adopted, with one refinement.

otherwise the overview tab has no valid answer even after a frontend fix.

That is the correct reading, and this Suggestion is what the blocker above was resolved with instead of the frontend plumbing the blocker prescribed.

Refinement. The split is made on whether the key names an existing slot, not merely on whether a key was sent. A request naming a real slot stays strict, so it still resolves to the same project chat_runner matches from slot.project, and a chat with no project of its own is told so rather than shown a neighbouring chat's checkout. Only a request that names no live slot takes the shared-project fallback.

Applying active_project_dir to the whole HTTP surface would have been the looser change: a projectless chat's prompt list would fill with a neighbour's local prompts, which that chat can never expand, and a local write from it would land in someone else's tree. Splitting on slot existence keeps the strict answer wherever a strict answer exists.

Pinned by test_a_named_slot_with_no_project_gets_no_neighbours_project (both list and create).

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • The five-handler HTTP migration ships with no consumer that can use it, converting "sometimes the wrong project" into "never any project" for the UI the title names.

Disposition: fixed — the premise is correct and independently confirmed. The prescribed subtraction is rejected; the concern it names is closed a different way.

I counted the callers of the five /api/prompts* endpoints: 5 API methods (client.ts:2549-2558), consumed at PromptsTab.tsx:134/140/159/246 and promptsProvider.ts:180, and zero pass a chat-slot key.

Confirmed in the three-dot diff and in the shipped client. This lane and Design Review converged on one concern, so it is answered once at its root rather than patched twice.

Fix. One seam, _prompt_local_project, now answers "which project does this request's local scope name" for all five handlers, and it splits by caller: an X-Session-Key naming an existing slot resolves strictly per-slot; a request naming no slot gets active_project_dir's single-shared-project fallback. The declared benefit becomes reachable for the surfaces that actually exist.

Rejected remedy, and why.

Subtraction: defer the five HTTP-handler changes (revert prompts.py to the prior resolver) until a caller exists that names a slot; land the chat_runner half - the per-slot resolution that actually reaches users - alone.

This would restore, not remove, the defect #7345 reports. Reverting prompts.py leaves the lister and the write resolver on _project_dir(), i.e. KIROCREW_PROJECT_DIR, which names the Kiro Crew source tree on a git install and nothing on a wheel. Landing chat_runner alone would then make the two halves disagree for the first time: the chat would match the user's own checkout while the Prompts tab listed and wrote into the Kiro Crew tree, so a prompt the user can see in the tab is one no @mention resolves and vice versa. The subtraction removes the reachability problem by removing the fix; the seam removes it by giving the fix a caller.

Pinned by the slotless-request and named-slot tests in test/test_prompts.py.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Watch: The description presents slot.project as what "the chat @mention surface asks", but the diff itself is what makes the chat surface ask it (chat_runner.py:2895, 5206) - the standard being "matched" is created by this PR and undeclared as a change.

Disposition: fixed — a documentation defect, and a worse one than the note claims.

The observation is exactly right, and checking it turned up a false factual claim rather than merely an undeclared one. On main, _find_prompt(raw_name) takes no project argument and its lister resolves the local directory from _project_dir(). So before this change both halves read the same process-wide KIROCREW_PROJECT_DIR: they did not disagree, and a chat @mention did not resolve against the user's own checkout. The PR description, the commit message and the spec paragraph all asserted the opposite, that the @mention "resolved fine" pre-change. That framing was wrong in the one direction that flatters the PR, by inventing a standard for the fix to be measured against.

Fix, documentation only, no code or test change:

  • The PR description's Problem section now states that every prompt surface read that one variable, names _find_prompt(raw_name) on main as the evidence, and says plainly that the halves "agreed on the same wrong answer" - then gives that as the reason both move together.
  • The commit message body carries the same correction and lists the @mention expander alongside the lister and the write resolver as a reader of the process-wide value.
  • docs/system-specs/modules/learn-cron-dashboard.md no longer records the false history; it states that the process-wide resolver is under neither checkout and that both halves resolve from the requester for that reason. A spec is the copy that outlives the PR, so a false prior-behaviour claim there is the most expensive of the three.

Items 1, 2 and 5 from the "What this change ships" list are now declared: the chat @mention and chat-side listing changes are named in both the description and the commit message, and the keyless-request behaviour is no longer described as failing closed to None, because it does not - a keyless request is precisely the slotless case that takes the shared-project fallback.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • App tokens can read foreign slot-local prompts (prompts.py:237)

Disposition: fixed, as prescribed.

Fix: For app requests, accept only an app-owned slot; otherwise resolve no local project

Legitimate, and introduced by this PR: before it, the prompt GETs ignored X-Session-Key entirely because the project came from a process-wide variable, so this change is what first let a header select which slot's project is read. An app token whose manifest grants /api/prompts reaches both GET handlers - the writes already refuse app tokens in _deny_non_owner_prompt_write - so a forged header disclosed a foreign project's prompt names and descriptions, and through the unscoped detail lookup their bodies. A reachable disclosure, so never waivable as disproportional.

Fix, inside the same _prompt_local_project seam: an app caller gets neither the shared-project fallback nor a foreign slot. It resolves only for a slot whose _app matches its own claim; every other case narrows to no local project. The app keeps the package SOPs and global prompts its path grant does cover and gains no local ones. Refusals are SEL-audited under source="app_isolation", best-effort so an unwritable SEL cannot turn the narrowing into a 500 or a leak.

Narrowing rather than a 404 is the one deliberate variation, and it is narrower than a refusal, not looser: the same response also carries package SOPs and global prompts, which are not slot-scoped, so a 404 would withdraw a capability the app's grant legitimately covers. Isolation is equivalent - no foreign slot's project is ever resolved for an app caller.

TestAppTokenLocalPromptIsolation pins six branches: no foreign-slot list; no foreign-slot resolve-by-name, asserting the body carries no content; no shared-project fallback, with a dashboard control proving the branch is reachable; own slot still reads; the refusal is audited; an unwritable SEL still narrows.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Use the active_project_dir fallback only for slotless dashboard requests

Disposition: fixed, as prescribed.

This is the same root concern Design Review and First Principles Review reached from the other direction - that with strict per-slot resolution on every handler, the shipped dashboard callers resolve to no project at all - and the remedy this lane names is the one implemented.

_prompt_local_project applies active_project_dir on exactly one branch: a non-app request whose X-Session-Key names no existing slot. A request naming a real slot keeps requesting_slot_project, so it still resolves to the project chat_runner matches from slot.project, and an app request gets no fallback on any branch.

The split is on slot existence rather than on header presence, which matters for the placeholder key the frontend sends: dashboard:ui normalizes to slot ui, so if no such slot is open it takes the fallback, and if one is open it is treated as that chat. Pinned by the slotless tests (empty key and dashboard:ui), by test_a_named_slot_with_no_project_gets_no_neighbours_project, and by the two-disagreeing-slots test that refuses no_active_project and writes nothing.

@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 Sep 2, 2026
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 09305b1 to 00752e6 Compare September 2, 2026 07:15
@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 Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Per-slot discovery follows repository symlinks into protected files (src/kiro_crew/dashboard/handlers/__init__.py:539, anchor backend-security-controls)

fixed — legitimate, and the chain is this PR's own. Fixed in 00752e6b07de2754280274b007f49c228de2222a.

Malicious project prompt symlink → listing or @mention_scan_prompt_dir follows it → protected file content is exposed.
Fix: Resolve each entry first and skip targets outside the prompt directory or matching is_sensitive_path.

Confirmed against the three-dot diff. The read-through-a-link shape predates the PR, but its reachability does not: on main the "local" half scanned agent._project_dir(), a process-global taken from KIROCREW_PROJECT_DIR. This PR points it at slot.project — a directory the user merely opened as a chat project, i.e. a checkout whose .kiro/prompts the repository's author writes. A cloned .kiro/prompts/creds.md -> ~/.aws/credentials was therefore newly reachable, and the consequence is worse than the listing: _extract_sop_description publishes the target's first heading, and @creds injected the whole file into an agent turn — a file the agent's own read gate refuses outright.

I closed the class rather than the call site. The invariant is now: a user prompt exists only when its name resolves to a plain file inside its own prompt directory, and every entry is minted by one helper. _prompt_dir_entry is that helper; _scan_prompt_dir and the new exact-name lookup both mint through it, so no consumer re-establishes the property and a future consumer cannot forget to. Its None is a refusal to name the file at all, not a missing description.

branch before after pinned by
project entry symlinked out of the prompt dir described + @mention-able not a prompt (containment) test_a_planted_symlink_out_of_the_dir_is_not_a_prompt, test_the_escaped_target_is_not_described_either
same, reached by @mention target injected into the turn not_found test_an_escaping_symlink_is_not_mentionable
same, reached by unscoped GET /api/prompts/{name} already 403 via validate_file_path 404, body absent test_an_escaping_symlink_is_not_readable_by_name
entry hardlinked to a file outside described + injected not a prompt (st_nlink > 1) test_a_hardlinked_prompt_is_not_a_prompt
entry resolving onto an is_sensitive_path target user half unguarded (package half already skipped) not a prompt, both halves test_a_sensitive_resolved_target_is_dropped_from_both_halves
@mention read of a path it was handed checked the name as addressed, read the target reads the path hooks.validate_file_path canonicalized, re-checked there test_the_mention_read_refuses_an_escaped_path_it_is_handed
scoped read / PUT / DELETE already refused a linked entry, hardlink, escaping root unchanged pre-existing tests still green

The last row is why I also changed chat_runner: leaving that read to trust "the lister filtered it" is the coupling that produces the next sibling. is_sensitive_path(match["path"]) there tested the name, not the file a read by that name returns.

Repo helpers, not a new matcher: containment is the resolved-to-resolved comparison _local_prompt_dir_in_project already uses, sensitivity is security.is_sensitive_path through the same module attribute the edition SOP walk 100 lines above already consults, and the read-site canonicalization is hooks.validate_file_path.

Widening re-checked for the opposite failure, since three of the refusals are new:

must NOT be refused pinned by
a dotfile-managed ~/.kiro symlink (resolved-to-resolved, not unresolved) test_a_dotfile_managed_kiro_dir_still_lists_its_prompts
an alias link that stays inside the prompt dir (containment, not "is a link") test_a_link_that_stays_in_the_directory_still_works
an unreadable (mode 000) prompt — keeps its entry, so a bad mode surfaces as the read path's own error instead of the prompt vanishing test_an_unreadable_prompt_keeps_its_entry + the pre-existing test_detail_unreadable
a symlinked project root pre-existing test_symlinked_project_root_itself_still_works

I deliberately did not fail closed on unreadability, which the literal "read it through the root-pinned gate" form would have done: it made a chmod-000 prompt disappear from the library with no error anywhere, and reverting that mutation is what test_an_unreadable_prompt_keeps_its_entry pins.

The rule is unconditional rather than project-only. It withdraws exactly one thing: a hand-symlinked individual prompt file under ~/.kiro/prompts no longer appears. That is a consistency gain, not a loss — the scoped read (is_link_or_junction → 403 access_denied) and both write verbs already refuse a linked entry in either scope, so the listing was advertising a file its own scope will not serve.

Each fix was proven by mutating the production code and confirming the test reds: dropping the containment check reds 4, dropping st_nlink reds 1, dropping is_sensitive_path reds 1, reverting the @mention canonicalization reds 1, and the two over-strict forms (refuse-any-link, unresolved-root comparison) red the two tolerance tests. Spec updated in the same commit (docs/system-specs/modules/learn-cron-dashboard.md).

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Prompt expansion performs an uncached filesystem scan on the event loop (src/kiro_crew/dashboard/chat_runner.py:2908, anchor no-blocking-call-on-event-loop)

fixed — and I took the remedy you named rather than the offload. Fixed in 00752e6b07de2754280274b007f49c228de2222a.

Large or network-backed prompt directory → chat turn → synchronous glob and per-file reads → gateway heartbeat stalls.
Fix: Use a bounded exact-name local lookup instead of scanning the project directory here.

The on-loop claim checks out: _expand_prompt_mention is called directly from async def _run_chat (chat_runner.py:5652, and again at :5162 for /prompts get) with no to_thread. What this PR added to that path is the newly-blocking part — the local half of the listing is keyed by the caller's project, so it is deliberately never cached, and _list_aim_prompts(project_dir) appended a fresh glob('*.md') plus one read_text per file on every turn beginning with @, in a directory the gateway does not own. On main a warm 5s cache meant zero filesystem I/O there.

Which fix, and why yours is the better one. asyncio.to_thread would have moved the scan off the loop while leaving it unbounded and uncacheable — and _expand_prompt_mention calls slot.append and state.push_slots_update(), so making it awaitable is a larger change than the defect warrants. The bounded lookup removes the scan: a local entry's name and fullName are both the file stem, so exactly one path in that directory can ever match a bare name. _find_prompt now searches the cached project-independent half by list and resolves the local half through _local_prompt_entry, which builds that one path and gates it. Cost on the loop goes from O(files in the project prompt dir) opens to one, and on a warm cache the base costs nothing.

branch before after pinned by
@mention resolving a local prompt enumerated <project>/.kiro/prompts on the loop one gated open, no enumeration test_resolving_a_mention_does_not_scan_the_project_dir
GET /api/prompts listing scans (correctly — it is the enumeration endpoint, in an executor) unchanged test_the_listing_still_scans_the_project_dir
stem present both globally and locally project-independent half wins (single list, global first) unchanged test_the_project_independent_half_still_wins_a_stem_collision
@sub/foo, @../escape, @/etc/passwd matched nothing (glob('*.md') yields direct children only) matches nothing (containment refuses it) test_a_name_that_is_not_a_direct_child_matches_nothing
@pkg/name could not match a user prompt (package is "") short-circuits, never touches the project test_a_packaged_spelling_does_not_probe_the_project
GET /api/prompts/%00 404 (no list entry matched) 404 — ValueError from path resolution is caught test_an_unencodable_name_is_a_miss_not_a_crash

The last three rows are the widening re-check you would expect from turning an enumeration into a path join: without them, @sub/foo would newly reach a file no listing shows, and a %00 in the URL path would have become an unaudited 500. The bounded lookup shares _prompt_dir_entry with the scan, so the containment check is what keeps only a direct child addressable — I dropped a redundant entry.parent != prompts_dir guard once a mutation showed the gate already covered every case, rather than shipping an unpinned duplicate.

scripts/check_sync_io_in_async.py: neither file is in .github/sync-io-in-async-baseline.txt, and the gate does not flag this defect — _expand_prompt_mention is a plain def, so lexical scanning of async def bodies cannot see it. The gate passes before and after (sync_io=0, 80 known calls in 3 unrelated files), and no baseline update was needed. Worth naming: the gate would not have caught this, which is why the review lane did.

Out of scope, stated rather than silently left: on a cold 5s cache the project-independent half still walks prompt_source_roots() with rglob('*.sop.md') on the loop from this same call site. That is unchanged from main and is not what this PR introduced; fixing it means making the expander awaitable, which is a separate change. My fix removes the uncacheable per-turn work this PR added and leaves the cacheable pre-existing walk exactly as it was.

Mutation-proven: restoring _list_aim_prompts(project_dir) in _find_prompt reds test_resolving_a_mention_does_not_scan_the_project_dir and test_a_packaged_spelling_does_not_probe_the_project; checking the local half first reds the collision test; dropping the ValueError guard reds the %00 test.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Successful app-slot authorization is not audited (src/kiro_crew/dashboard/handlers/prompts.py:231, anchor backend-security-controls)

fixed — small, unambiguous, and correct. Fixed in 00752e6b07de2754280274b007f49c228de2222a.

App requests its owned slot → ownership check grants local prompt access → no SEL grant event is emitted.
Fix: Emit an allowed log_api_access event before returning.

The branch is this PR's (_prompt_local_project is new in the three-dot diff) and it was asymmetric: the refusal emitted outcome="denied", the grant emitted nothing. That is the wrong half to keep. The ownership test is the whole authorization for an app reading another principal's checkout through this endpoint, so after a compromised app — where every selection succeeded — a log carrying only refusals cannot say which project the app was served. An operator would be reconstructing reach from the absence of denials.

I copied the shape from the neighbouring authorized path rather than inventing one. _deny_foreign_app_skill_slot in the same file already emits, on its allow branch, log_api_access(caller=request_app, operation=<op>, outcome="allowed", source="app_isolation", resources=f"slot={slot_name}") inside a try/except that logs at debug and proceeds. Mine is field-for-field that, with the same operation="prompt_local_project" and source="app_isolation" the denial below already uses — no new event name, and sel.md's event_type table is unchanged (api_access). One app request now leaves exactly one attributable line under one operation/source pair, whichever way the selection went.

branch before after pinned by
app names a slot it owns served, unaudited served, outcome="allowed" naming the slot test_a_granted_slot_selection_is_audited
app names a foreign / unknown slot outcome="denied" with a reason unchanged pre-existing test_refused_slot_selection_is_audited
SEL unwritable on the grant path n/a access is preserved, not withdrawn test_an_unwritable_sel_still_serves_the_granted_slot
SEL unwritable on the denial path narrowing holds unchanged pre-existing test_an_unwritable_sel_still_narrows_the_answer
dashboard request (no app claim) unaudited here unchanged — no app boundary was crossed

The third row is the widening re-check: making the audit fail-closed on the grant path would let an unwritable SEL withdraw an access the ownership check authorized, so the except clause matters as much as the emission. Narrowing the except to a type SEL does not raise reds that test.

Two judgement calls worth stating rather than leaving implicit. I emit allowed on the owned-slot branch unconditionally, including when that slot has no project of its own: whether the slot is project-bound is not an authorization outcome, and calling it denied (as the skills-surface precedent does, because its contract is a 404) would put a refusal in the log where none happened. And I did not add a second event for the dashboard path — no app boundary is crossed there, and a per-request line for every dashboard prompt read is noise that would bury the app lines this finding is about.

Mutation-proven: removing the emission reds test_a_granted_slot_selection_is_audited; narrowing its except reds test_an_unwritable_sel_still_serves_the_granted_slot. docs/system-specs/modules/learn-cron-dashboard.md now documents both outcomes in the same commit.

@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 Sep 2, 2026
Comment thread src/kiro_crew/dashboard/chat_runner.py Fixed
Comment thread src/kiro_crew/dashboard/handlers/__init__.py Fixed
Comment thread src/kiro_crew/dashboard/handlers/__init__.py Fixed
Comment thread src/kiro_crew/dashboard/handlers/__init__.py Fixed
Comment thread src/kiro_crew/dashboard/handlers/__init__.py Fixed
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 00752e6 to e7e9dbd Compare September 2, 2026 07:39
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 8cdcd87 to 2a23ca1 Compare September 4, 2026 01:01
@bolichen97

bolichen97 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author
  • src/kiro_crew/dashboard/handlers/prompts.py:521 — Local prompt reads bypass the pinned no-link gate (span=6f77c3fb4ba7) — fixed in 2a23ca179, at both sites the finding names.

return p, path.read_text(...) / "description": _extract_sop_description(path)
Swapped local prompt -> detail/list reopens its path -> sensitive file contents reach the dashboard.
Anchor: backend-security-controls
Fix: Read through safe_read_file_bytes_nolink with the prompt root and size cap.

The finding holds, and I reproduced the leak rather than reasoning about it. With the by-name reads in place and a hardlink planted in <project>/.kiro/prompts, the unscoped GET /api/prompts/{name} answers 200 with the outside file's bodyassert 200 != 200 is the pre-fix failure of the new test. Same for an inode outside the scope root. The listing's description read reaches the target too, one heading or description: value at a time. So this is the same defect class already accepted and fixed on the @mention path in this PR, on the two readers that were still by-name — a completeness gap in my own previous round, not a new demand.

Why the minting gate does not already cover it. _prompt_dir_entry refuses a link and a hardlink by lstat, so a link never survives minting. What a by-name read still loses is the window BETWEEN that lstat and the open: the path is re-resolved there, so an entry swapped in between serves bytes no check ran against. And O_NOFOLLOW alone would not be enough either — it guards only the final component, so an ancestor directory swapped for a link escapes with the leaf looking ordinary. That is what within_root is for.

The table of every reader of a prompt entry, since narrowing one branch of a chain is how the next sibling arrives a round later:

Reader Gate Status
chat @mention (_resolve_prompt_mention) safe_read_file_bytes_nolink, entry-derived root already fixed earlier in this PR
scoped GET ...?scope= (_api_user_prompt_detail._read) safe_read_file_bytes_nolink, within_root=target_dir already correct on main; addressed by scope, not by entry, so it derives its own root legitimately
unscoped GET /api/prompts/{name} safe_read_file_bytes_nolink, entry-derived root fixed here
the listing / lookup description read (_prompt_dir_entry) _gated_sop_description → same primitive, pinned to prompts_dir fixed here
write verbs (PUT/DELETE) hooks.verified_replace_file_nolink already correct
package SOP walk (_extract_sop_description) by-name, deliberately roots come from the platform seam, not a checkout; unchanged, and that reader is now the ONLY caller of the by-name helper

One root derivation, not one per reader. _prompt_read_root(entry, project_dir) is now the single source of the within_root every reader passes, and chat_runner's inline copy was replaced by a call to it — a root derived per reader is precisely how two readers of one directory come to disagree. It answers a root only for an entry positively naming local or global (the scopes whose root the minting gate already validated the entry against) and None for a package SOP or an unfamiliar source, which keeps O_NOFOLLOW and the hardlink check without inventing a containment that would newly refuse legitimate edition-contributed SOPs.

Two deliberate non-changes, so the fix is not read as wider than it is.

A refused or unreadable description yields an EMPTY description, never a dropped entry. _prompt_dir_entry documents that a bad mode or a transient I/O error must surface as the read path's own error rather than a prompt silently vanishing from the user's library, and that contract is preserved.
The unscoped read's refusals collapse onto the file not readable 500 it already answered for an unreadable file. The gate reads and refuses through ONE descriptor, so it cannot report which of a link, a hardlink, an escaping inode or a bad mode happened — and separating them would make the endpoint an existence oracle for a link's target, the same reason the scoped read answers one code for a live and a dangling link. Mapping them onto the existing 500 also keeps test_detail_unreadable's contract intact; the sensitive-path refusal is still checked BEFORE the read and keeps its own coded access denied 403, so that outcome stays separately audited.

Tests are in TestEveryPromptReaderUsesTheNoLinkGate, which carries the table above in its docstring. The listing test makes _extract_sop_description RAISE, so a user-prompt entry can neither route back through the by-name reader nor pass by having quietly stopped reading; _gated_sop_description is pinned against a symlink (under requires_symlinks), a hardlink and an outside inode, and against a plain contained prompt so the refusals are not the only outcome. I also widened the bounded-lookup description counter to count BOTH readers, so moving the read back cannot make its "exactly one, for the matched prompt" bound look satisfied by having stopped observing it. The spec paragraph records the closed reader set, the shared root derivation and the collapsed status, in the same commit.

@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 Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • src/kiro_crew/dashboard/handlers/__init__.py:730 — Local prompt root validation has a directory-swap race (span=a0c697372730) — needs-a-decision: I judge the remedy out of proportion to this PR and I have no override authorization, so I am asking rather than either fixing it or dismissing it.

base.extend(_scan_prompt_dir(local_dir, "local")) / with os.scandir(prompts_dir) as entries:
Concurrent prompt-root swap -> validated path is reopened -> files outside the project are listed or injected by @mention.
Anchor: residual/security
Fix: Pin the validated root and enumerate/open children relative to its descriptor.

The mechanism is real and I am not disputing it. _resolve_prompt_dir("local", ...) validates the root (leaf link check plus resolved-to-resolved containment) and then _scan_prompt_dir / _local_prompt_entry re-open that root BY NAME, so a swap landing in between is enumerated. It is the same class as the leaf swap I fixed this round, one level up the path, and the lane's proposed remedy is the correct one.

Why I am escalating instead of fixing.

It is a residual gap in a defence this PR ADDS, not a regression. On main the listing does no root validation and no entry validation at all — _list_aim_prompts globs <proj>/.kiro/prompts/*.md and calls _extract_sop_description on each, following any link freely (origin/main:handlers/__init__.py lines 586-605). A statically-planted .kiro/prompts -> ~/Documents is published there with no race required. This PR closes that, plus the entry-level link/hardlink/containment refusals and now the read gate. So on this exact axis the branch is a large net improvement, and what is left needs an attacker already running code as the user at request time — at which point they can read the target file directly and this endpoint is not the boundary that matters. The lane classifies it residual/security itself.

The remedy is a re-architecture, not a localized change. Reads would have to move onto dir_fd-relative enumeration the way the WRITE verbs already do via _pin_prompt_dir, which means reworking _prompt_dir_entry (shared by three callers, and currently Path-based: resolve(), is_link_or_junction, stat, _gated_sop_description), _scan_prompt_dir and _local_prompt_entry, plus a NEW descriptor-based reader — hooks.safe_read_file_bytes_nolink takes a path, not a dir_fd, so the description and body reads cannot reuse it under a pinned design — plus the _DIR_FD_SUPPORTED = False by-name fallback for Windows and its own test matrix mirroring TestCreateAndDeletePinTheDirectory. That is a larger and more security-critical change than everything else in this round, landing on top of an already ~2,600-line diff, and it is a new primitive rather than an application of an existing one.

The question for you. Do you want (a) this deferred to a follow-up PR that moves the prompt READ paths onto the pinned-descriptor walk the write verbs already use — I will open the issue with owner and due date, noting that the skill's rules say a security finding is not deferrable without your ruling, which is exactly why I am asking — or (b) it done here, accepting that this PR grows by a new descriptor-based reader plus a Windows fallback and a second review cycle on that? I have no preference I would act on unilaterally; I lean (a) because the branch already strictly improves this axis and the remaining step is self-contained. I have made no code change either way so your ruling is not pre-empted.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • src/kiro_crew/dashboard/handlers/prompts.py:270 — App authorization fails open when SEL is unavailable (span=6f77c3fb4ba7) — needs-a-decision: the remedy reverses a contract this PR established one round earlier at this lane's own request, and contradicts the module's pre-existing convention, so it is a policy ruling rather than a fix I should make unilaterally.

except Exception: / return requesting_slot_project(state, session_key)
Failed SEL write -> owned-slot app request -> local prompt access succeeds without the mandatory audit event.
Anchor: backend-security-controls
Fix: Use a critical audit and return None when it fails.

The mechanism is accurate. If log_api_access raises, the app still receives its own slot's project and no allowed event is written.

What makes it a decision rather than a fix.

The authorization itself does not depend on SEL. The gate is the ownership test slot._app == request_app, evaluated before the audit and unaffected by it; the app receives only the project of a slot it OWNS. So an unwritable SEL costs the audit record, not the isolation — a foreign or unnamed slot still narrows to None on the branch below, which the lane is not disputing.

The current behaviour is deliberate, documented and pinned by a test. test_an_unwritable_sel_still_serves_the_granted_slot asserts exactly it: "an unwritable SEL must not withdraw an access the ownership check authorized." Adopting the remedy means inverting that test. And the audit being there at all is this lane's own earlier finding on this PR — "Successful app-slot authorization is not audited" — so the two rounds pull in opposite directions and only a maintainer can say which one wins.

It is the module's established convention, not something this PR invented. _deny_non_owner_skill_trust and _deny_foreign_app_skill_slot are both on main with the identical shape and the identical # noqa: BLE001 — preserve authorized access if SEL is unwritable comment. Flipping only this one call site would leave the prompt surface failing closed while the two sibling gates on the skills surface fail open — an inconsistency that is worse than either policy applied uniformly, and unifying them is a change to main's security posture well outside this PR.

Failing closed adds an availability failure mode with an attacker-reachable trigger. A full disk or an unwritable SEL directory would silently withdraw every app's access to its own local prompts. Anyone able to fill the disk could therefore turn an audit outage into a capability outage.

The question for you — three options, and I will implement whichever you pick:

  1. Keep as-is (preserve access, audit best-effort) — I rebut the finding on the grounds above and nothing changes.
  2. Make the failure loud but non-denying — raise logger.debug to logger.error at both call sites so an unwritable SEL is visible in the gateway log. This addresses the "silently unaudited" half with no contract change, no test inversion and no new outage mode. This is the option I would choose if it were mine.
  3. Fail closed as the lane asks — return None on an audit-write failure. If you pick this, I would want it applied to the two sibling gates in the same change so the convention stays uniform, and test_an_unwritable_sel_still_serves_the_granted_slot inverted with its docstring rewritten to state the new contract. That is a deliberate widening of this PR and a posture change on main, which is why I am not doing it on my own judgement.

I have made no code change, so your ruling is not pre-empted in any direction.

@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 Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

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 #7181 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7181: KEEP. Distinct fixes on one shared field, mutually reinforcing rather than redundant. No duplicated code. Files: src/kiro_crew/dashboard/chat_runner.py.
  • This PR is OVERLAPPING with PR #8249. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7715: KEEP. Same author, same file, deliberately stacked: PR #8249's own body states it exists because PR #7715's siblings were dispositioned 'accepted-and-deferred', and it defers its site 4 until PR #7715 lands. Land 7715 first, then rebase 8249 onto it; keep both. Files: src/kiro_crew/dashboard/handlers/prompts.py, test/test_prompts.py.

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

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 2a23ca1 to 90e8bc7 Compare September 5, 2026 03:59
@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 Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

GPT 5.6 Review, round on 2a23ca17 — both findings answered

2a23ca17 is superseded by 90e8bc75 (rebase onto f59b865bb + the fixes below).

1. BLOCKING — "Local prompt root validation has a directory-swap race" — fixed in 90e8bc75

Accepted as described. _resolve_prompt_dir validated a path, and every containment
decision downstream then re-resolved that same name: _prompt_dir_entry's
resolved.parent != prompts_dir.resolve() comparison, and the within_root its
description read is pinned inside. With the root replaced by a link after validation,
both sides of every later comparison resolve into the link's destination, so every file
under the directory the swap named looked confined — published with a filename and
first-heading description by GET /api/prompts, and injectable in full by @<stem>.

The fix pins the value, not a descriptor. prompts._local_prompt_scan_root runs the
existing gate and then returns the root resolved once, and _prompt_dir_entry /
_scan_prompt_dir / _gated_sop_description compare against that fixed value instead of
re-deriving it:

  • a swap landing before the pin makes the pinned value escape the project, which the
    containment gate refuses;
  • a swap landing after it leaves every entry's resolved parent unequal to the pinned
    root, so the scan and the exact-name lookup both mint nothing.

So the local library goes empty under an active swap — the same answer a statically
redirected root already gives, and the same one every serving verb answers
linked_prompt_root for. The global root is pinned for the duration of its scan too,
which costs that scope nothing.

On the suggested descriptor pin specifically: it would additionally keep the honest
entries listed through a swap, but os.scandir(fd) / dir_fd do not exist on Windows
(prompts._DIR_FD_SUPPORTED is already False there), so it buys availability on one
platform only, while the refusal above is what carries the security property on all
three. The write verbs keep their real descriptor pin (_pin_prompt_dir), where the
operation is a mutation and a refusal is not an acceptable substitute.

Pinned by TestARootSwappedAfterValidationPublishesNothing, which injects the swap
deterministically by wrapping the function that takes the pin — no timing. Verified to
fail against the previous behaviour: status == 'ok' and the planted body resolved, i.e.
the leak reproduced, then refused.

2. BLOCKING — "App authorization fails open when SEL is unavailable" — not applied, and here is the concrete reason

The finding is accurate about the mechanism and I am not disputing the reading of the
code. I am disputing that critical=True + return None is the right answer here, on
three grounds that are all checkable in the tree:

a. This is not the authorization. The authorization is the line above it —
slot is not None and getattr(slot, "_app", "") == request_app. An app that reaches the
allowed branch is being served a slot it owns. SEL cannot make that ownership test
truer or falser; the audit exists so an operator can reconstruct which project an app
was served after a compromise, which is why the PR added the allowed event at all.
Failing closed converts an operator-side storage failure into a capability withdrawal
for data the grant does cover
— the app silently loses its own project's local prompts
while keeping global and package ones.

b. critical=True is scoped, by its own contract, to a different class of audit.
sel.py's log_api_access docstring: "Pass critical=True for fail-closed audits (e.g.
safety-override activation): the event is written synchronously and a filesystem failure
is re-raised so the caller can refuse the audited action."
A read narrowing on an
owned slot is not a safety-override activation. It is also synchronous-write-on-the-loop:
_prompt_local_project is the one step of this handler deliberately left on the event
loop precisely because it touches state._slots and nothing else. A critical audit puts a
synchronous, lock-taking filesystem write back on the loop — the thing
no-blocking-call-on-event-loop and this PR's own off-loop split exist to prevent.

c. It would make this the only gate on the surface that behaves that way. Both
pre-existing app-isolation gates in the same module, untouched by this PR and on main
today, use the identical best-effort shape with the identical comment on their allowed
branch:

  • prompts._deny_non_owner_skill_trustexcept Exception: # noqa: BLE001 — preserve authorized access if SEL is unwritable
  • prompts._deny_foreign_app_skill_slot — same comment, same allowed branch, then proceeds

Adopting fail-closed for _prompt_local_project alone would leave three sibling gates on
one endpoint family with two different answers to "SEL is unwritable", which is how the
next reader picks the wrong one. If fail-closed audit-or-deny is the policy we want for
app-isolation grants, it belongs in one change that moves all three plus the SEL contract
— not as a divergence introduced by a prompt-scoping fix.

What the PR does keep is the property the finding is really protecting: both outcomes
are audited under one operation/source pair, so an app request leaves exactly one
attributable line either way and a forged X-Session-Key is visible. That is pinned by
TestAppTokenLocalPromptIsolation, including that the refusal survives an unwritable SEL.

Happy to be overruled on (2) — it is a policy call about all three gates, not about this
diff.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 force-pushed the fix/7345-per-slot-prompt-resolution branch from 90e8bc7 to 0f7d1a2 Compare September 5, 2026 04:22
@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 Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Round on 90e8bc75 — the residual is real and is fixed in 0f7d1a2d9

BLOCKING -- prompts.py:452 -- Local prompt reads re-resolve an unpinned root

Correct, and thank you — the previous round pinned the root for the listing and
the exact-name lookup
but left the two readers deriving their own. Both
_prompt_read_root branches assembled a fresh, unresolved root
(project_dir / ".kiro" / "prompts", and Path.home() / ".kiro" / "prompts"), and
hooks.safe_read_file_bytes_nolink realpaths whatever within_root it is handed —
so the chain was re-traversed at read time and a swapped root resolved into the link's
destination on both sides of the comparison. An outside file carrying the matched
prompt's own name then passed the fd-real-path check.

Reproduced before fixing, on a swap injected at validate_file_path (the step
immediately before the read on both surfaces, so no timing is involved) with a legitimate
creds.md already in the project:

FAILED ...::test_the_unscoped_read_is_not_redirected_by_a_root_swapped_after_the_mint
E  assert b'OUTSIDE-ROOT-HEADING' not in b'{"name": "creds", ..., "source": "local",
   "content": "# OUTSIDE-ROOT-HEADING\n[REDACTED: credential]\n", "redacted": true}'
FAILED ...::test_a_mention_read_is_not_redirected_by_a_root_swapped_after_the_mint
E  assert 'ok' == 'not_found'

The unscoped read returned the outside file's body in full and the @mention resolved
ok. Both now refuse.

The fix. _prompt_read_root answers a user scope's root resolved through that
scope's own gate
_local_prompt_scan_root for local, the followed-and-resolved
~/.kiro/prompts for global (that root is operator-chosen, so the documented listing
asymmetry is preserved). A resolved value leaves no link in the path for the gate's
realpath to re-traverse.

That makes None mean refuse, not "unconstrained", for a user-scope entry — which is
the second half of the finding, because falling back to the canonical path's own parent in
that state pins the read inside precisely the directory the swap named. One shared
derivation, _prompt_read_within_root, now owns both the root and the fallback so the
two answers cannot drift: user scope → the pinned root, or None → refuse (the
@mention's ordinary miss, the unscoped read's file not readable 500); package SOP or
an unfamiliar entry shape → the canonical parent, which is the only root available across
plural seam-supplied roots and the one thing that carries the guarantee onto Windows,
where O_NOFOLLOW does not exist and only GetFinalPathNameByHandleW sees the
substitution.

On the alternative you named — storing the pin on the entry: the entry dict is what
GET /api/prompts serialises, so a root_real key would publish the resolved prompt root
to the client and would need stripping at every serialisation boundary. Deriving it from
the same gate the mint used gives the identical value without a field that has to be
remembered.

Pinned by four tests in TestARootSwappedAfterValidationPublishesNothing (mint-time swap
for the listing and the lookup, read-time swap for the @mention and the unscoped read),
plus test_a_user_scope_read_with_no_serveable_root_is_refused_not_widened and a
test_the_read_root_is_derived_once_for_every_reader that now asserts the root comes back
RESOLVED, against a project reached through a symlink so the two spellings are
distinguishable at all. Every one verified to fail when its own branch is reverted.

The SEL finding from the previous round is no longer raised; the reasoning I gave for it
stands in the comment above, and I am still happy to be overruled on it as a policy call
across all three app-isolation gates.

@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Round on 0f7d1a2d9 — finding 1 is correct and I am stopping short of the fix, deliberately

BLOCKING — prompts.py:503 — "Resolved path does not pin the prompt root"

You are right, and my previous answer was wrong in its reasoning. I claimed a
"resolved" root leaves no link to re-traverse. It does not: the value is still a name,
and hooks.safe_read_file_bytes_nolink does root_real = os.path.realpath(within_root)
at read time (hooks.py:2387), so a root swapped between _prompt_read_root returning
and that realpath is followed on both sides of the containment comparison. In the
honest case <project>/.kiro/prompts resolves to itself, which is exactly why the value
pin looked equivalent and why my two new tests passed for the wrong reason — the swap in
them lands before _prompt_read_root, so the gate catches it rather than being immune
to it.

So the accurate statement of where this stands:

  • Closed this round: the window from the mint to the read. It used to span the whole
    scan and the whole lookup (a swapped root published every *.md under the directory it
    named, with descriptions, and made them @mention-able), and the readers used to derive
    a root that was never gated at all. Both now re-run the scope's gate, and the two tests
    in this round demonstrate the leak reproducing ("content": "# OUTSIDE-ROOT-HEADING\n…"
    returned by the unscoped read; status == 'ok' on the @mention) and then being refused.
  • Still open, as you say: the window between that gate and the gate's own
    realpath + open. Two adjacent syscalls rather than a whole scan, but a real TOCTOU,
    and a name-based within_root cannot close it.

Why I am not fixing it in this PR. Your suggested fix — hold a directory descriptor
through validation and read, opening entries relative to it — is the right one, and it is
a change to hooks.safe_read_file_bytes_nolink's contract, not to this call site.
That primitive takes a within_root path and has ~40 consumers across the tree,
including two on this very surface that are main's and not this diff's:

…plus memory.py:819, skills.py:1978, themes.py:366/434, webapp_preview.py:378,
kiroignore.py:199, cron.py:832/836, and the rest. Every one of them has the identical
residual, because it is a property of the primitive.

Adding dir_fd to that function and threading a pinned_fs.open_dir_pinned descriptor
through the prompt readers is a coherent change and I think it should happen. It is not
this change: doing it here would either (a) reshape a keystone security primitive inside a
2,900-line prompt-scoping fix, or (b) hand-roll the gate's fstat/nlink/_fd_real_path/
is_sensitive_path sequence at these three call sites — the duplication pinned_fs's own
charter exists to prevent ("callers stay thin consumers of one set of invariants") — and in
either case it would land descriptor semantics on three readers while thirty-odd others,
including the scoped read beside them, keep the name-based contract. That is the same
"one gate on this surface behaves differently from its siblings" problem I argued against
for the SEL finding, and I do not think I get to invoke it in one direction only.

_DIR_FD_SUPPORTED in this module is already False on Windows, so the descriptor form
also needs a documented name-based fallback there — which is a policy decision about what
the guarantee is per platform, not a mechanical edit.

So: your blocking verdict stands, and it should. What I am asking for is a maintainer
call between:

  1. /ai-review override gpt 0f7d1a2d91fea94d1d0e869189a90a8ddfa53950: <reason> — accept
    the two-syscall residual as the same one every other consumer of the primitive carries,
    and land the scoping fix, which strictly reduces the exposure it inherited; or
  2. hold this PR for a follow-up that adds dir_fd to
    hooks.safe_read_file_bytes_nolink, threads a pinned descriptor through every prompt
    reader (mention, unscoped detail, scoped detail, both description readers), and settles
    the Windows fallback — at which point this PR rebases onto it and the residual is gone
    for the whole surface rather than for three call sites.

I am not making that call myself and I have not requested an override.

BLOCKING — prompts.py:271 — "App authorization proceeds after its audit fails"

Re-raised; my answer is unchanged and is in the comment above. In one line: the
authorization is the ownership test on the line before it, the app is being served a
slot it owns
, sel.py's own docstring scopes critical=True to "fail-closed audits
(e.g. safety-override activation)", _prompt_local_project is the one step of this
handler deliberately left ON the event loop, and both pre-existing app-isolation gates in
this same module (_deny_non_owner_skill_trust, _deny_foreign_app_skill_slot, on main
and untouched here) use the identical best-effort shape with the identical comment on
their allowed branch. If audit-or-deny is the policy we want, it belongs in one change
that moves all three. Also a maintainer call, and also one I am not making.

Prompt `local`/"This project" scope resolved through the process-wide
`KIROCREW_PROJECT_DIR`, which is auto-detected from repository markers:
it names the Kiro Crew source tree on a git install and nothing at all
on a wheel. Either way it is not the user's project, so a prompt
authored in their own checkout was invisible to every prompt surface
alike — absent from the Prompts tab, not creatable or editable there,
and unmatchable by `@mention`.

One seam, `_prompt_local_project`, now answers "which project does this
request's `local` scope name" for all five prompt HTTP handlers, so
list, scoped read, create, update and delete cannot disagree: a prompt
created there is one the same request lists. A request naming an
existing slot resolves strictly per-slot; a slotless one gets the
single shared project, which is what gives the overview tab and the
command palette an answer at all. The `dashboard:ui` placeholder folds
to "names no slot" before any parser turns it into a real slot name. An
app token gets neither the fallback nor a foreign slot, and both
outcomes of that selection are SEL-audited. `chat_runner` passes
`slot.project`, so a chat matches its own checkout.

Making an untrusted cloned `.kiro/prompts` reachable is what pulls the
safety envelope in: one entry gate mints every user-prompt entry and
refuses a linked one outright, the local prompt ROOT is gated too (an
entry check cannot see a redirected directory), and the `@mention` read
goes through the hardlink-rejecting `safe_read_file_bytes_nolink`, so
the inode checked is the inode injected.

Both halves of the surface do their filesystem work off the event loop,
split by which THREAD may do it. `_resolve_prompt_mention` is handed a
message and a project directory — never the slot or the state — and
runs in `asyncio.to_thread`; the chip it returns is appended back on
the loop, because `slot.append` ends in `slot.event.set()` on an
`asyncio.Event` whose waiters resolve through the loop's `call_soon`
rather than its threadsafe variant. On the HTTP side the unscoped
detail GET resolves and reads inside one executor job, so the `stat`
and `read_text` that used to follow the match cannot stall the gateway
on storage it does not own.
@bolichen97

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 41909a3: both findings describe residuals that live outside this diff — the path-confinement window is imposed by hooks.py's realpath call, byte-identical on main, not in the diff, and shared by ~40 call sites in 31 files (closing it needs a new dir_fd-relative hooks primitive plus a Windows fallback decision), and the SEL best-effort block is byte-identical on main at prompts.py:81 and :133 where the authorization is the ownership test on the preceding line, not the audit.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14.

both findings describe residuals that live outside this diff — the path-confinement window is imposed by hooks.py's realpath call, byte-identical on main, not in the diff, and shared by ~40 call sites in 31 files (closing it needs a new dir_fd-relative hooks primitive plus a Windows fallback decision), and the SEL best-effort block is byte-identical on main at prompts.py:81 and :133 where the authorization is the ownership test on the preceding line, not the audit.

This decision applies only to this commit. A new push requires a new judgment.

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.

Per-chat-slot project resolution for the prompt 'This project' scope

3 participants