fix(dashboard): per-chat-slot prompt 'This project' resolution (#7345) - #7715
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] 41909a3 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. The sole candidate — [OPUS-REVIEWED] 41909a3 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All checks are done: consumers counted, existing mechanisms verified ( First-Principles-Verdict: CONCERNS The fix is cause-level and honest, but it ships a third named copy of What this change shipsIntent: make "This project" prompts resolve to the requesting chat's own checkout instead of the process-wide install-tree variable (#7345) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 41909a3 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @bolichen97 overrides the GPT 5.6 finding for 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: |
53cae2e to
09305b1
Compare
Disposition: fixed — the concern holds exactly as written. The prescribed remedy is rejected and the lane's own Suggestion adopted in its place.
Verified against the shipped frontend rather than taken on trust: Fix. All five handlers resolve through one seam, Rejected remedy, and why.
There is no correct slot for those two surfaces to name, and the trailing clause is the reason. Plumbing a Pinned in |
Disposition: fixed — adopted, with one refinement.
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 Applying Pinned by |
Disposition: fixed — the premise is correct and independently confirmed. The prescribed subtraction is rejected; the concern it names is closed a different way.
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, Rejected remedy, and why.
This would restore, not remove, the defect #7345 reports. Reverting Pinned by the slotless-request and named-slot tests in |
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 Fix, documentation only, no code or test change:
Items 1, 2 and 5 from the "What this change ships" list are now declared: the chat |
Disposition: fixed, as prescribed.
Legitimate, and introduced by this PR: before it, the prompt GETs ignored Fix, inside the same 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.
|
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.
The split is on slot existence rather than on header presence, which matters for the placeholder key the frontend sends: |
09305b1 to
00752e6
Compare
fixed — legitimate, and the chain is this PR's own. Fixed in
Confirmed against the three-dot diff. The read-through-a-link shape predates the PR, but its reachability does not: on 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.
The last row is why I also changed Repo helpers, not a new matcher: containment is the resolved-to-resolved comparison Widening re-checked for the opposite failure, since three of the refusals are new:
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 The rule is unconditional rather than project-only. It withdraws exactly one thing: a hand-symlinked individual prompt file under Each fix was proven by mutating the production code and confirming the test reds: dropping the containment check reds 4, dropping |
fixed — and I took the remedy you named rather than the offload. Fixed in
The on-loop claim checks out: Which fix, and why yours is the better one.
The last three rows are the widening re-check you would expect from turning an enumeration into a path join: without them,
Out of scope, stated rather than silently left: on a cold 5s cache the project-independent half still walks Mutation-proven: restoring |
fixed — small, unambiguous, and correct. Fixed in
The branch is this PR's ( I copied the shape from the neighbouring authorized path rather than inventing one.
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 Two judgement calls worth stating rather than leaving implicit. I emit Mutation-proven: removing the emission reds |
00752e6 to
e7e9dbd
Compare
8cdcd87 to
2a23ca1
Compare
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 Why the minting gate does not already cover it. 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:
One root derivation, not one per reader. 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. Tests are in |
The mechanism is real and I am not disputing it. Why I am escalating instead of fixing. It is a residual gap in a defence this PR ADDS, not a regression. On The remedy is a re-architecture, not a localized change. Reads would have to move onto 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. |
The mechanism is accurate. If What makes it a decision rather than a fix. The authorization itself does not depend on SEL. The gate is the ownership test The current behaviour is deliberate, documented and pinned by a test. It is the module's established convention, not something this PR invented. 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:
I have made no code change, so your ruling is not pre-empted in any direction. |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
2a23ca1 to
90e8bc7
Compare
GPT 5.6 Review, round on
|
90e8bc7 to
0f7d1a2
Compare
Round on
|
Round on
|
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.
|
/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. |
Human judgment recorded@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Closes #7345.
Blocked on a decision, not on a fix (
GPT 5.6 Review)GPT 5.6 Reviewis the only red check on41909a3f; 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 aKNOWN RESIDUALblock. The window is imposed by the shared primitive, not by this PR:hooks.safe_read_file_bytes_nolinkresolveswithin_rootby name at read time (hooks.py:2387), and that line is byte-identical onmain—hooks.pyis not in this diff. The primitive has ~40 call sites across 31 files, andmain's ownprompts.pyalready passes a name-basedwithin_rootat 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 thatrealpath, 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 — asafe_read_file_bytes_nolinkcontract 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_SUPPORTEDis False and there is nodir_fdread 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, returnNone) 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_trustand_deny_foreign_app_skill_slotcarry the byte-identical best-effort shape, with the identical# noqa: BLE001 — preserve authorized access if SEL is unwritablecomment, both already onmain(prompts.py:81and:133there) and both untouched by this diff. It also runs against the repository's own stated doctrine for the flag:critical=Trueis 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:
/ai-review override gpt 41909a3fc3886378e3fdfaf4385fec5c4c6e9f14: <reason>(repository writer, keyed to the current head), if the analysis above holds; ordir_fdcontract change onhooks.safe_read_file_bytes_nolinkfirst — 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/promptsstops 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-wideKIROCREW_PROJECT_DIR. That variable is auto-detected from theagents/+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
@mentionexpander. Onmain,_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 answerscannot_resolveon every wheel install) andfiles.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:971and theupdates.py/terminal_commands.pyuses 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'slocalscope 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._list_aim_prompts,handlers/__init__.py): split into_build_prompt_base(edition SOPs +~/.kiro/promptsglobals — project-independent, so still cached under the 5s TTL) plus_scan_prompt_dirfor 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._user_prompt_dir, its wrapper_resolve_prompt_dir, and the confinement check_local_prompt_dir_in_project,prompts.py): all take the caller-resolvedproject_dirinstead of reaching for the process-wide one, so the symlink-confinement check is made against the very root the write was addressed to.chat_runner.py):@mentionexpansion and the chat-side prompt listing passslot.project, so a chat matches its own checkout's prompts.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@mentionon 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_mentiontakes the message and a project directory, is handed neither the slot nor the state, and goes toasyncio.to_thread; the "Loaded prompt" chip it returns is appended by_surface_prompt_chipback on the loop, after theawait. Both call sites go through_expand_prompt_mention_off_loop, which does exactly that, and_expand_prompt_mentionstays as the synchronous on-loop entry point for callers already on that thread.awaitkeeps 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 putslot.appendon a worker, and that is not safe: it ends inslot.event.set()on anasyncio.Event, whose waiters are resolved through the loop'scall_soonrather 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.prompts.py):api_prompt_detail's unscoped branch offloaded_find_promptand then finished on the loop —hooks.validate_file_path, the sizestatand the bodyread_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_readalready 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_projectis the only step left on the loop, and it readsstate._slotsand nothing else.dashboard:uiplaceholder is not a slot (prompts.py): it is folded to "names no slot" before_named_slotor 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 intoui, 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.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 withplatform_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~/.kirokeeps working. Every filesystem call is wrapped andRuntimeErroris caught alongsideOSError/ValueError, becausePath.resolve()raisesRuntimeErroron a symlink loop: a cloned project shippingloop.md -> loop.mdloses one entry instead of takingGET /api/promptsdown with a 500.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_rootdocuments. So the local scope's two enumerating callers (_list_aim_promptsand_local_prompt_entry) resolve the root through the same helper the scoped read and both write verbs use, and treat alinked_prompt_rootanswer 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*.mdunder the directory it named published byGET /api/prompts, and@<stem>would inject up toMAX_PROMPT_BYTESof one — while every serving verb answeredlinked_prompt_rootfor the same name.is_sensitive_pathdoes 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 anOSError— and there the loop needs no race to arrive:_linked_prompt_rootasksos.path.islink, which swallows theELOOPand answers False, so a checkout shipping a cyclic.kiroreaches_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 onGET /api/promptsand 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/promptswhile 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.@mentionread (chat_runner._expand_prompt_mention): the read is performed on the pathhooks.validate_file_pathcanonicalized, 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 throughhooks.safe_read_file_bytes_nolink— the same gate the scoped read uses: it opens FIRST withO_NOFOLLOWand 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 > 1is the only signal a second name for a sensitive inode leaves, and it is readable only on the descriptor).within_rootis passed only when the entry positively names thelocalorglobaluser 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 samenot_foundan unreadable file already produced, so it reveals nothing a link's target could be probed with.prompts.py,handlers/__init__.py): the minting gate refuses a link and a hardlink bylstat, so what a by-name read still lost was the window BETWEEN thatlstatand 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 ordescription:value). Both now go throughhooks.safe_read_file_bytes_nolink, joining the@mentionexpansion and the scoped read, so the set of readers is closed. A user-prompt entry describes itself through_gated_sop_description;_extract_sop_descriptionis 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 naminglocalorglobal, andNonefor a package SOP or an unfamiliar entry shape, keepingO_NOFOLLOWand 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 thefile not readable500 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 ownaccess denied403._local_prompt_entry): the candidate is taken from the directory's own entry name (os.scandir, onegetdents, no per-entrystat) rather than by joining the caller's string onto the prompt root. The two spellings name the same inode —_plain_stem_okruns 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'slstatand tworesolves, onegetdents, 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@mentionopens that one file twice and a miss opens nothing. Stated honestly as a delta: againstmain'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 costThe branch was 160 commits behind. Three upstream changes overlap it and each is
reconciled rather than overwritten:
fix(security): read prompt paths through the descriptor gateconverged onthe same design as this PR's unscoped detail read —
hooks.safe_read_file_bytes_nolinkinstead of a by-name
read_text. Its version does the read ON the loop after theexecutor resolved the match; this PR's does it inside the same
_resolve_and_readjob.The executor form is kept, and fix(security): read prompt paths through the descriptor gate #8249's two properties are carried onto it: the
within_rootfalls back to the canonical path's own parent when_prompt_read_rootanswers
None(a package SOP), because that fallback is what carries the guarantee ontoWindows, where
O_NOFOLLOWdoes not exist and only the fd-real-path check sees thatthe inode opened is not the one resolved; and
_gated_sop_descriptiongains fix(security): read prompt paths through the descriptor gate #8249's_audit_unreadline for a withheld description, so routing user prompts through adifferent reader does not silently withdraw that audit.
allow_truncaterather than a byte cap. Thisbranch had capped
_gated_sop_descriptionatMAX_PROMPT_BYTESand returned an emptydescription past it; that is now
allow_truncate, for fix(security): read prompt paths through the descriptor gate #8249's reason — a description isfrontmatter or a first heading, both at the head of the file, so a truncated read
answers the same question while a raising cap costs an oversized prompt a description
the by-name read gave it.
fix(skills): refuse writes to kiro-user/ and kiro-workspace/ keysrewrote thespec's adjacent Skills (CRUD) paragraph. The conflict is resolved as main's Skills
paragraph plus this branch's Prompts paragraph; nothing of fix(skills): refuse writes to kiro-user/ and kiro-workspace/ keys #8259's text is dropped.
Two Windows-only test splits, also from the rebase.
hooks.validate_file_pathgaineda 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
realpathbelow it into the outbound SMBprobe its lexical UNC gates exist to prevent, and Windows has no
O_NOFOLLOWto fall backon. 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_resolvessplit into three.The root gate's tolerance (a
.kirolink that stays inside the project still LISTS) isplatform-neutral and keeps its exact assertion; the mention RESOLVING it is now POSIX-only,
and the Windows outcome is pinned positively as the
blockedthat screen produces, withfirst_linked_ancestorandvalidate_file_pathasserted directly so the refusal isattributed to that gate and cannot be mistaken for a root-gate regression.
test_the_unscoped_read_is_not_redirected_by_a_root_swapped_after_the_mintasserts 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 — thewithdrawal 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
blockedaudit outcomemoves 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 aratchet against tightening the description read into a blanket link refusal. That
ratchet is preserved — it now asserts
_gated_sop_descriptionstill describes a linkwhose 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.
prompts._local_prompt_scan_root): the root is resolved once by the gatethat approved it, and
_prompt_dir_entry/_scan_prompt_dir/_gated_sop_descriptioncompare 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.
prompts._prompt_read_within_rootover_prompt_read_root): bothreaders were still assembling a fresh, unresolved
<project>/.kiro/promptsas theirwithin_root, andhooks.safe_read_file_bytes_nolinkrealpaths what it is handed — sothe 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@mentionresolvedok. A user scope now gets its root resolved through that scope'sown gate, and
Nonethere means REFUSE rather than "unconstrained" — a fallback to thecanonical 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_rootis a PATH andhooks.safe_read_file_bytes_nolinkrealpaths it at readtime, so a root swapped between the derivation and that
realpathis 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 thewrite verbs already use), which is a
safe_read_file_bytes_nolinkcontract changeshared with its other ~40 consumers, including the scoped read (
prompts.pyline 1185 onmain) and the package-SOP description read (#8249, line 289) on this same surface, whichcarry the identical residual. Plus a decision about the name-based fallback on Windows,
where
_DIR_FD_SUPPORTEDis already False. That belongs in one change that moves everyreader, 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/promptsreachable 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:~/.kiro/promptsno longer appears. With~/.kiro/prompts/{plain.md, stowed.md -> ~/dotfiles/stowed.md, alias.md -> plain.md},mainlistsalias,plain,stowedand this branch lists onlyplain. So a GNU Stow / chezmoi per-file link and an intra-directory alias both vanish from the Prompts tab and stop resolving for@mention— onmain@stowedexpanded, 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.kirolinked to another directory inside the same project are all ancestor links, and all keep working (pinned).linked_prompt_rootfor 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/promptsat 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:
X-Session-Keynames an existing slot speaks for one chat, so it resolves strictly per-slot (requesting_slot_project) — the same answerchat_runnerreaches fromslot.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.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 shareddashboard:uiplaceholder, which is folded to the same slotless answer, so a strict-only resolver would answerNonefor every request they can make and "This project" would be permanently dead there. Two chats on different projects still resolveNone; a settings page has no defensible answer then, and guessing would create, overwrite or delete in the wrong checkout. That flows into the existingno_active_projectcontract — no new error codes.uiresolves as the slotless surface too. Its real key isdashboard:<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 arbitraryX-Session-Keywould 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 underapp_isolationwithoperation=prompt_local_project— anallowedevent names the slot an app was actually served, adeniedone 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-Keyselects one slot's project across a two-slot statea slotless dashboard request reaches the single shared project, for both the empty and
dashboard:uikeys, and a create from it lands therea slotless request with two slots on different projects refuses
no_active_projectand writes nothinga 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 SELa 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.mddrops one entry and not the library: the listing keeps its siblings,GET /api/promptsstill answers 200, and the@mentionlookup is a missthe
@mentionread 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 namethe 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
@mentionin a directory of 13 prompts extracts exactly one description, so the per-turn work does not grow with directory sizeTestPromptExpansionStaysOffTheEventLoop, 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.rundrives 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, overchat_runner's own source, because the@mentionsite 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 functionTestEveryPromptReaderUsesTheNoLinkGate, 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_descriptionmade to raise, so the entry cannot silently route back through the by-name reader nor pass by having stopped reading;_gated_sop_descriptionrefuses a symlink (underrequires_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_rootis pinned per entry shape, including that an unfamiliarsourcefalls back to no root rather than a wrong onethe 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 itTestUnscopedDetailReadStaysOffTheEventLoop:hooks.validate_file_path— the first step after the match, with thestatandread_textbehind 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 overapi_prompt_detail's coroutine body (nesteddefs 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 getstatus stubs intest_chat_runner_coverage.pyand the re-entry identity test intest_active_turn_session_key.pynow 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 measuresTestTheDashboardPlaceholderKeyNamesNoChat: with a slot nameduiand a neighbour on a different project, the placeholder key lists none of that slot's local prompts, alocalcreate refusesno_active_projectand writes nothing, and alocaldelete 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 brancha redirected prompt ROOT publishes nothing, in both shapes (
.kiro/prompts -> <outside>and.kiro -> <outside>), asserted as the agreement: the listing has nolocalentries, the@mentionmisses and leaks no body, and the scoped read's ownlinked_prompt_root403 is pinned alongsidea
.kirolink that stays inside the project still lists and still resolves, so the root gate is containment rather than a ban on linksan ANCESTOR symlink loop (
.kiroas a two-link directory cycle) costs the local library and not the request:GET /api/promptsanswers 200 with the global half intact and nolocalentries, the unscoped detail lookup is a 404, the@mentionis a miss, and the gate's ownlinked_prompt_rootanswer is pinned platform-neutrally by raising the loop directly. The cycle is planted with real symlinks underconftest.requires_symlinks:_winapi.CreateJunctionchecksGetFileAttributesWon its target before creating the link, so the target must already resolve and no junction can close a cycle — the junction fixture failed onwindows-latestwithWinError 2instead of planting one.CreateSymbolicLinkWhas 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.12Path.resolve()mapsERROR_CANT_RESOLVE_FILENAMEto the sameRuntimeErrorPOSIX raises forELOOPthe 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 otherthe 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 bitsExisting 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-loopRuntimeError, and the enumerated candidate behind the bounded local lookup with its honest cost. Updated for thatdashboard:uicounts as naming no slot (and why a chat nameduiresolves the same way), and — this round — for where the loop boundary actually falls on both halves of the surface: that expansion offloads_resolve_prompt_mentionwhile_surface_prompt_chipappends back on the loop (with theasyncio.Eventreason it must), and that the unscoped detail GET does all of its filesystem work in one_resolve_and_readjob. It also now records that the set of readers going throughsafe_read_file_bytes_nolinkis closed — the two by-name holdouts, the unscoped read and the listing's description read, included — the single_prompt_read_rootderivation behind each read'swithin_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'sTestARootSwappedAfterValidationPublishesNothing, which against the re-resolving version reportsstatus == '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 Lintreproduces onorigin/mainitself, 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-historyon a pristinef59b865bbworktree fails withtest/test_atomic_write_named_duplicates.py:155:/home/téstand: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 ownpath.resolve()andis_sensitive_path(str(resolved))— the containment gate that decides whether the entry may be named at all — and atchat_runner's prompt read, whose pathhooks.validate_file_pathhas already canonicalized and refused on — and which now reads throughhooks.safe_read_file_bytes_nolinkrather than by name. TheCodeQLandAnalyze (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 functionalmost always also touches loop-owned state — an
asyncio.Event, a queue, an SSEwriter, cached state the loop mutates — and that state is now mutated from a
worker. It fails silently (a
call_soonthe loop is never woken for) or onlyunder 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 realidentity 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 realprincipal 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.