fix(security): read prompt paths through the descriptor gate

[bolichen97]

Problem / Motivation

While #7715 was in review, the GPT lane raised the same defect class three
times under three names: by-name reopen of a validated, repository-supplied
prompt path. The shape is always the same — a prompt path is resolved and
validated once, and then a LATER step re-opens it by name, so what is
finally read is not provably the thing that was validated.

#7715 fixed the most severe instance (the @mention read, whose bytes reach an
agent turn as instructions) and its three siblings were dispositioned
accepted-and-deferred on the owner's ruling that they land in an independent
PR. This is that PR. This PR's own First Principles lane then found a fourth
instance in the same file, which is fixed here too, and a fifth in
skills.py, which is examined below and deliberately left alone. Three of the
five are fixed; one is deferred because it sits inside code #7715 rewrites, and
one is a tolerated by-name read of operator-installed content whose hardening
would cost more than it buys (both below).

The concrete, no-race exploit for every fixed site is a hardlink. It shares
its target's inode, so realpath yields the alias's own innocent path,
is_symlink() is False, O_NOFOLLOW has no link to refuse, and
is_sensitive_path sees an ordinary file sitting inside the directory it was
found in — while the bytes belong to whatever it aliases. st_nlink is the only
signal a second name for a protected inode leaves, and it is readable only on
an open descriptor.

Why it matters

A project's .kiro/prompts holds content the user CLONED, and anything with
write access to a checkout (including the agent itself) can plant
ln ~/.aws/credentials <project>/.kiro/prompts/notes.md. Before this PR:

• GET /api/prompts published that file's first # comment line as the
  prompt's description (an INI-style credentials file and ~/.ssh/config
  both use # comments, and a YAML file can supply a frontmatter description:).
• the unscoped GET /api/prompts/{name} returned its whole body, up to
  MAX_PROMPT_BYTES.
• GET /api/skills/package/<name> did the same for a capability-seam package
  skill root, with no size bound at all.

All of those are files the agent's own read gate refuses outright, served
through dashboard endpoints that had already "validated" the path.

What changed (motivation → approach → change)

The five sites, each verified against the real diff

#	Site (on origin/main)	Verdict
1	handlers/prompts.py:193 _extract_sop_description — path.read_text() after every caller has already judged the path (handlers/__init__.py:579 for package SOPs, :602 for user prompts). Cited by the GPT lane as handlers/__init__.py:630 in #7715's numbering.	fixed
2	handlers/prompts.py unscoped api_prompt_detail — validate_file_path(p["path"]) then Path(resolved).stat() + Path(resolved).read_text(). Cited as prompts.py:443.	fixed
3	handlers/prompts.py api_skill_detail's package/ branch — validate_file_path(row["path"]) then Path(resolved).read_text(), the exact pre-fix shape of site 2. Found by this PR's own First Principles lane.	fixed
4	The local prompt enumeration — handlers/__init__.py:586-607, prompts_dir.glob("*.md") against a root _resolve_prompt_dir validated by name. Cited as prompts.py:360.	deferred until #7715 lands
5	skills.py:2433-2437 load_skill's _extra_paths branch — validate_file_path(skill_file) then Path(resolved).read_text(). Raised by the First Principles lane on the second revision.	examined — left alone

What the fix does

All three fixed reads now go through hooks.safe_read_file_bytes_nolink, the
gate #7715 already routes the @mention read and the scoped read through. It
opens first with O_NOFOLLOW, then fstats that one descriptor and refuses
st_nlink > 1, a non-regular inode, and an is_sensitive_path target — so the
inode validated is the inode served. This is an existing repo mechanism with
existing precedent on exactly this shape (docs/system-specs/features/steering-viewer.md
documents the same hardlink-in-a-listing fix for the steering surface); no new
matcher, no new helper, and no existing check is weakened or removed.

within_root is passed on all three, as the canonical path's own parent. The
first revision of this PR omitted it, arguing that a root derived from the
resolution being defended authorizes nothing. That reasoning was right about what
it authorizes and wrong about what it is for: on Windows O_NOFOLLOW does
not exist at all and the gate asks for it with getattr(os, "O_NOFOLLOW", 0), so
there the open FOLLOWS a leaf swapped for a link after canonicalization and the
fd-real-path check (GetFinalPathNameByHandleW) is the only thing left that can
see the inode opened is not the one resolved. A link the entry legitimately points
at is already followed by validate_file_path, so its target's own directory IS
the root and only a substitution landing after that resolution escapes it.
Deterministically pinned by two tests that delete os.O_NOFOLLOW (the gate reads
it with getattr at call time, so removing the attribute reproduces Windows'
open semantics on any host) and inject the swap inside the gate's own
validate_file_path call, so no timing is involved.

The refusals report differently per surface, on purpose — in every case as the
outcome that surface already produced for a file it could not open, so a refusal
is indistinguishable from I/O trouble and no endpoint becomes an oracle for
whether a given path is protected.

• A refused description yields an empty description and keeps the entry.
  Whether a prompt exists is its caller's decision, not this function's, and a
  library that dropped a file because its metadata was refused would hide a name
  the scoped read still serves. That also keeps an unreadable prompt (bad mode,
  transient error) listed with no description, exactly as the by-name read did.
• A refused unscoped prompt read answers the same 500 file not readable an
  unopenable file already produced. No new response body and no new status code,
  so no new code field is owed.
• A refused package-skill read leaves content unset, which is the existing 404.

Bounds. The unscoped read's MAX_PROMPT_BYTES cap moves off a pre-read
stat and onto the gate's own max_bytes, so the size that refuses is the size
of the bytes actually read rather than of a separately-stat'd name;
FileTooLargeError is not an OSError, so each site catches it explicitly — the
prompt read to keep its coded 413, the skills read so the gate's default 50 MB
ceiling cannot escape as an unaudited 500 where an unbounded read_text
previously succeeded. The description read passes allow_truncate instead of a
cap, because a description is frontmatter or a first heading — both at the head
of the file — and raising there would turn one oversized file into a 500 for the
whole listing, something the unbounded by-name read it replaces could not do.

One tidy-up that is not incidental: validate_file_path moves from two
function-local imports to the module-level hooks import. That is load-bearing
for the tests above — the handlers must hold their own binding so patching
hooks.validate_file_path reaches only the gate's internal call, which is the
window being simulated — and it retires the two stale # noqa: F811s.

Second-revision fixes: the refusals are audited, and the 50 MB read is off the loop

The GPT lane blocked the second revision on two things that are not about the
gate but about what surrounds it, and both are fixed rather than argued.

Every refusal now writes one coarse SEL line (prompts._audit_unread).
The indistinguishable-response argument above is about the HTTP response and
holds only there: SEL is operator-side and unreachable through the endpoint.
Without a line there the primary exploit is not merely refused, it is invisible
— an entry listing with an empty description is byte-identical to a prompt that
simply has none, and a 404 is what an uninstalled skill name produces, so the
planted alias this PR exists to stop would leave the operator nothing to find.
The line says THAT bytes were withheld and never why: the gate judges and reads
through one descriptor and answers a bare None, so a refused inode and an
ordinary read failure are the same value, and re-stating the path to separate
them would be another by-name look at exactly the input these reads stopped
trusting. blocked is carved out as the one knowable cause, because
validate_file_path refuses the name before any open; otherwise the surface's
own outcome (error, or too_large for the skills read). Best-effort, so an
audit write can never fail a listing or a detail view. The Design lane raised the
same gap independently; recording the gate's internal reason would need
safe_read_file_bytes_nolink to return one, a contract change shared with its
other consumers and wider than this PR.

The package-skill read runs under asyncio.to_thread. No caller-supplied cap
applies to it, so it reads up to the gate's own 50 MB default off storage that can
be network-backed, on the one loop every other session's turn shares — and the
same handler already routes its DELETE and PUT verbs and its kiro-user/ GET
branch off the loop for that reason. The two prompt reads stay on it deliberately:
both are bounded (max_bytes=MAX_PROMPT_BYTES = 100 KB, and allow_truncate for
the description), so the gate reads at most read_limit + 1 bytes and a hop would
buy nothing. Nothing about the gate's guarantees moves with the call — the open,
the fstat, the st_nlink refusal and the fd-real-path check all still happen on
one descriptor inside one call.

Site 5: examined, and left alone on purpose

The lane is right that skills.py:2437 has the same by-name shape, and the site
is exactly where it says. What does not hold is that its neighbours treat that
class of root differently. skills.py:2468 calls
_read_enumerated_skill_bytes(skill_file, _within, ...), whose FIRST branch is
if within is None: return path.read_bytes() — and its docstring names the
members of that branch outright: "the global skills dir, extra paths, edition
roots, and the paths writers construct themselves … these are operator-installed,
so there is no directory to confine them to". So the neighbour is hardened only
when a PROJECT grant supplies a root; handed an _extra_paths root it takes the
same by-name read. There is no inconsistency inside skills.py to repair, there
is a documented decision with named costs: taxing that branch with the hardened
reader "measurably slowed the per-message listing path (test_skill_listing_cost
guards it) and emptied frontmatter on Windows".

The actor differs too, and that is the load-bearing half. Site 3's path arrives
from CapabilityManager.list_skills() at request time; _extra_paths is resolved
once at loader construction from the operator's own cfg.skills.extra_paths plus
mcp_tooling.extra_skills(). Nothing a checkout supplies reaches it, and the
entry served is <extra>/<name>/SKILL.md with name already past _safe_name.
Its immediate sibling one branch above (self._dir / name / "SKILL.md") reads
with no validate_file_path at all, so the _extra_paths branch is already the
stricter of the two.

Applying this gate there would also trade an unreachable regression for a
reachable one. An edition contributes roots that can sit inside site-packages,
and a wheel installed by uv on Linux lands with st_nlink == 2 by default
link-mode, so a blanket st_nlink > 1 refusal would empty legitimate,
operator-installed skill bodies on ordinary installs. The prompt sites carry no
such risk: nothing installs a ~/.kiro/prompts/*.md or a checked-out
.kiro/prompts/*.md, and the scoped read on main already refuses st_nlink > 1
in both user scopes — so this PR made the listing and the unscoped read agree with
a refusal that was already there.

Site 4: why it is deferred, not skipped

The remedy for the enumeration is right and the primitive already exists in this
file (_pin_prompt_dir, which the create and delete verbs use): scandir
against the pinned descriptor instead of the name. It is deferred purely for
coordination. On main the enumeration lives in _list_aim_prompts'
user-prompt scan at handlers/__init__.py:586-607, and #7715 replaces every one
of those lines (its @@ -583,31 +767,10 @@ hunk) with _scan_prompt_dir /
_prompt_dir_entry / _local_prompt_entry — functions that do not exist on
main. Fixing it here would rewrite exactly the lines #7715 rewrites, producing
a guaranteed conflict and a confusing double-fix, and there is no way to land the
pinned form on main with a non-test caller in the shape #7715 is about to
delete. It follows #7715, against its own _scan_prompt_dir and
_local_prompt_entry.

Its residual is also the narrowest of the four, which is why it is the one left:
the actor in the chain is an agent with write access to the project, and that
actor can already get content listed and @mention-ed by writing a real *.md
into .kiro/prompts — which the gate is designed to allow. What the root swap
adds is aiming the enumeration at an EXTERNAL directory it did not author.

Coordination with #7715

This branch is cut from origin/main and its hunks are deliberately disjoint
from #7715's. Verified mechanically, not by eye:
git merge-tree --write-tree --messages HEAD refs/pull/7715/head merges clean
(exit 0, no conflict) across every shared file. chat_runner.py is untouched
here on purpose — #7715 owns that whole read. The **Prompts (CRUD)** line in
learn-cron-dashboard.md is likewise untouched (#7715 replaces that entire
single-line paragraph); the new spec text is a separate Prompt and
package-skill read safety paragraph three lines below it. An earlier revision
did conflict on one import line in test/test_prompts.py; that import moved into
the helper that uses it and merge-tree is clean again. Whichever lands second
needs at most a trivial rebase.

Tests

Nine new tests in test/test_prompts.py::TestPromptReadsGoThroughTheDescriptorGate
and four in test/test_skill_browser.py::TestPackageSkillDetailReadsThroughTheGate.
Each planted prompt lives in a cloned checkout's .kiro/prompts, which is the
untrusted half.

test	pins
test_a_hardlinked_prompt_publishes_no_description	site 1 — the alias keeps its listing entry with description == "", the secret appears nowhere in the listing, and the ordinary neighbour keeps its own description (the refusal narrows metadata, never the library)
test_a_hardlinked_prompt_is_not_served_by_the_unscoped_read	site 2 — 500, no secret bytes in the body, SEL outcome="error"
test_a_prompt_resolving_onto_a_sensitive_target_publishes_no_description	site 1's sensitivity half — a symlink into a sensitive store is still listed but described from nothing
test_a_leaf_swapped_after_validation_publishes_no_description	site 1's within_root, in the Windows shape (os.O_NOFOLLOW deleted)
test_a_leaf_swapped_after_validation_is_not_served_by_the_unscoped_read	site 2's within_root, same shape
test_a_hardlinked_package_skill_is_not_served	site 3 — 404, no secret bytes in the body
test_a_prompt_symlinked_to_an_ordinary_file_still_describes	tolerance, not the fix. The gate canonicalizes before it opens, so O_NOFOLLOW refuses nothing about a link the user chose. Green before the fix too; it is here so a later round cannot tighten the description read into a blanket link refusal without going red. Labelled as such in its docstring.
test_an_ordinary_package_skill_is_still_served	the same tolerance for site 3 — an unaliased SKILL.md still returns 200 with its content
test_a_prompt_at_exactly_the_cap_is_still_served	the boundary did not move when the cap moved from a pre-read stat onto the gate's max_bytes
test_a_refused_description_leaves_an_audit_line	the SEL line for a refused description, and that its ordinary neighbour produces none — a log that fires for everything says nothing
test_a_sensitive_description_target_is_audited_as_blocked	blocked, the one cause that IS knowable because it precedes any open
test_a_refused_package_skill_leaves_an_audit_line	the same for site 3, where the 404 is otherwise what an uninstalled name produces
test_the_package_skill_read_runs_off_the_event_loop	asserted as a shape, not a duration: the gate is wrapped, the thread it runs on recorded, and the loop's own thread must not be among them — so it cannot flake on a shared runner

Proven by reverting, not asserted. Every fix was reverted in the real file
and the tests re-run, then restored:

revert	reds
_extract_sop_description back to path.read_text()	test_a_hardlinked_prompt_publishes_no_description, test_a_prompt_resolving_onto_a_sensitive_target_publishes_no_description
the unscoped read back to stat + read_bytes	test_a_hardlinked_prompt_is_not_served_by_the_unscoped_read, with assert 200 == 500 — i.e. the reverted code hands the credential file back with HTTP 200
within_root= dropped from both prompt reads	both test_a_leaf_swapped_after_validation_* tests
api_skill_detail back to Path(resolved).read_text()	test_a_hardlinked_package_skill_is_not_served
the three _audit_unread calls removed	test_a_refused_description_leaves_an_audit_line, test_a_sensitive_description_target_is_audited_as_blocked, test_a_refused_package_skill_leaves_an_audit_line, test_detail_unreadable
await asyncio.to_thread(...) back to a direct call	test_the_package_skill_read_runs_off_the_event_loop

Nothing existing was weakened. test_detail_too_large (413,
outcome="too_large") stays green unchanged, which is what pins that the
response taxonomy did not shift. test_detail_unreadable (chmod 000 → 500) is
the one existing test this PR edits, and the edit tightens it: it asserted
log_tool_invocation.assert_called_once(), and a mode-000 prompt now genuinely
withholds TWO reads — the resolution walks the listing, whose description read is
refused by the same bad mode — so it now enumerates the lines by tool_name and
asserts api_prompt_detail == ["error"] and api_prompts == ["error"]. It
names each lane instead of counting all calls, and it reds when either audit line
is removed. The hardlink tests pytest.skip on a filesystem that cannot create a
second link; the symlink tests carry @requires_symlinks, the runtime capability
probe, rather than a platform guess.

Manual verification

N/A — unit coverage sufficient. The change is a swap of one read primitive for
another on three backend code paths, and both the exploit and the tolerated cases
are reproducible from a planted inode, which is exactly what the tests do (real
os.link and real os.symlink, no mocked filesystem). The Windows-only branch is
covered by removing the attribute the gate itself probes at call time, rather than
by claiming a platform run.

Related Issues

no linked issue: this is the follow-up split out of #7715 by the repository
owner's ruling, tracked by that PR's accepted-and-deferred dispositions rather
than by an issue of its own.

Pattern harvest

This is now the fifth instance of one class found in one file across five review
rounds, which is the definition of generalizable. The second lesson is separate
and cost this PR a round on its own: a descriptor gate whose leaf protection is
O_NOFOLLOW has NO leaf protection on Windows, so within_root is not optional
hardening there — it is the whole guard.

Rule candidate: review-prompt
Pattern: a path validated by NAME is re-opened by that same name for the read, so the bytes served are not the bytes checked — audit for read_text/read_bytes/open on a path that a resolve, lstat or is_sensitive_path above it already judged, route it through hooks.safe_read_file_bytes_nolink (the only way to see st_nlink, the one signal a hardlink leaves), and always pass within_root, because on Windows O_NOFOLLOW does not exist and the fd-real-path check is the only leaf guard left.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A recurring, named defect class closed with the repo's existing gate; scope, refusals, deferrals, and the tolerated site are each argued with evidence and pinned by revert-verified tests.

[DESIGN-REVIEWED] f364767

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f364767

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] f364767

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of f364767e2d603695d0c7f5c4a95d28bbae0e3932 — 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 verification is done. Composing the review.

Verification summary: the gate (hooks.safe_read_file_bytes_nolink, hooks.py:2238) is a pre-existing mechanism with 30+ consumers, so the fix reuses rather than invents; module-local _audit_* wrappers are the repo idiom (agent_discovery.py:148, cron.py:686, memory.py:568, etc.), so _audit_unread is not a second spelling; but the by-name-reopen class has undispositioned siblings beyond the declared five (chat_runner.py:1243, taskrunner.py:218), and the audit-on-refusal invariant leaves the steering precedent silent (steering.py:242, :618).

First-Principles-Verdict: CONCERNS

Every fixed site reuses the existing gate at cause level, but the defect class and the new audit-on-refusal invariant both have counted, undispositioned siblings.

What this change ships

Intent: stop three dashboard reads from re-opening a validated prompt/skill path by name — a FIX.

1. Listing description no longer drawn from a hardlink-aliased file — justified
2. Unscoped GET /api/prompts/{name} no longer serves an aliased or swapped body — justified
3. GET /api/skills/{name} package branch likewise — justified
4. Refused reads now write a SEL line (_audit_unread) — declared rider; siblings silent
5. Package skill over 50 MB now 404s where it was served — declared changed default
6. Package-skill read moves off the event loop — declared rider
7. Prompt 413 measured on bytes read, not a pre-read stat; boundary test-pinned — justified
8. Spec paragraph in learn-cron-dashboard.md — mandated (AGENTS.md same-commit rule)
9. validate_file_path hoisted to module import — declared, load-bearing for tests
10. Description read truncates instead of failing the listing — justified

Watch

• The audit-on-refusal invariant item 4 introduces ("a silent fallback is what makes a planted alias invisible" — _audit_unread's own docstring) has 2 unfixed siblings in the surface this PR cites as precedent: steering's listing read returns _empty_meta() (steering.py:242) and its scoped read returns readfailed (steering.py:618) on the same gate refusal, with no SEL line — grep log_tool_invocation in steering.py: 2 hits (:995, :1157), neither on a refusal path.
• The description's "five sites" table reads as the class's census, but the validate-then-reopen-by-name shape exists undispositioned at least twice more: _safe_read_snapshot (chat_runner.py:1234→1243, p.read_text after validate_file_path, bytes published into dashboard diff chips) and _read_spec_prefix (taskrunner.py:1298→218, bare open after validation). Accepted-and-deferred is fine — but these two were never examined.

Subtractions

• Shrink the twice-repeated ~25-line Windows/within_root rationale: keep it in _extract_sop_description's docstring (prompts.py:230-258) and reduce the near-verbatim comment blocks in api_prompt_detail (prompts.py:392-417) and api_skill_detail (prompts.py:2419-2444) to one-line pointers — four divergent copies (plus the spec paragraph) of one argument.

[FIRST-PRINCIPLES-REVIEWED] f364767

[bolichen97]

• src/kiro_crew/dashboard/handlers/prompts.py:234 — Windows symlink swaps bypass the descriptor gate (span=6f77c3fb4ba7) — fixed

raw = safe_read_file_bytes_nolink(str(path), allow_truncate=True)
body_bytes = safe_read_file_bytes_nolink(resolved, max_bytes=MAX_PROMPT_BYTES)
Windows prompt-entry swap -> O_NOFOLLOW is unavailable and no within_root triggers descriptor-path validation -> a sensitive symlink target is read and exposed.
Anchor: residual/security
Fix: Pass each pre-open canonical parent as within_root to both calls.

Legitimate, and it refutes the PR body's own reasoning rather than the code. Fixed in 96091b3f7d317a923c1a63d58bf892fb97d5ed87 exactly as asked: both calls now pass within_root=os.path.dirname(<canonical>), and so does the third instance the First Principles lane named in api_skill_detail.

I omitted within_root deliberately and argued it in prose — that a root derived from the resolution being defended authorizes nothing. That is true about what it authorizes and beside the point about what it is for. hooks.safe_read_file_bytes_nolink reaches for the leaf guard as getattr(os, "O_NOFOLLOW", 0), which is 0 on Windows, so on that platform the open follows a leaf swapped for a link after validate_file_path canonicalized it, fstat sees an ordinary regular file with st_nlink == 1, and the pre-open is_sensitive_path was asked about the wrong inode. The fd-real-path check (GetFinalPathNameByHandleW) is then the only thing left that can observe the substitution. The chain holds as written.

Verified against the real three-dot diff, not the finding text: both quoted lines are added lines of this PR, so this is this PR's own residual and not pre-existing main code.

Why the derived root is still the correct root, which is the part worth stating rather than leaving to the next round. A link the entry legitimately points at is followed by validate_file_path before the root is computed, so the root is the target's own directory and a pre-existing alias out of the prompt tree is unaffected — pinned by test_a_prompt_symlinked_to_an_ordinary_file_still_describes and test_an_ordinary_package_skill_is_still_served, which stay green. Only a substitution landing after that resolution escapes, which is precisely the window.

Pinned deterministically, with no timing. test_a_leaf_swapped_after_validation_publishes_no_description and test_a_leaf_swapped_after_validation_is_not_served_by_the_unscoped_read delete os.O_NOFOLLOW (the gate reads it with getattr at call time, so removing the attribute reproduces Windows' open semantics on a POSIX host) and inject the swap inside the gate's OWN validate_file_path call, which runs immediately before the open. That required moving validate_file_path to a module-level import in the handler, so patching it in hooks reaches only the gate's internal call and leaves each handler's pre-open resolution — the one that supplies the root — untouched. Dropping within_root= from the two prompt reads reds exactly those two tests; reverting api_skill_detail reds test_a_hardlinked_package_skill_is_not_served.

Opposite failure mode re-checked, since within_root fails CLOSED when the fd's real path cannot be determined: the same within_root shape is already in production on the steering viewer's per-document listing read and the pptx-maker's asset reads, so the /proc/self/fd and F_GETPATH dependency is not new to this codebase. The spec paragraph in learn-cron-dashboard.md now says plainly that the root is the canonical parent and not an authorization, so the next reader does not have to re-derive why it is passed.

[bolichen97]

• First Principles 🟡 CONCERNS — one undeclared sibling of the same pattern at api_skill_detail — fixed

src/kiro_crew/dashboard/handlers/prompts.py:2354-2358 — api_skill_detail's package-skill branch does validate_file_path(row["path"]) then Path(resolved).read_text(), the exact pre-fix shape of item 2. … Route it through the gate or disposition it the way site 3 was.

Legitimate, reproduced, and fixed in 96091b3f7d317a923c1a63d58bf892fb97d5ed87 rather than dispositioned. The lane is right that it is the same five lines this PR had already written twice, and right that the actor requirement is weaker (write access to a capability-seam package skill root rather than to a cloned checkout) — but the defect is identical and a hardlink defeats it with no race and no link at all, so is_sensitive_path judges the alias's own innocent path while the bytes are the aliased file's.

Why fixing beat deferring here, given site 4 was deferred. The two deferrals are not the same kind of thing. Site 4 cannot land because #7715 deletes the exact lines it would rewrite, so there is no shape to land it in; this one is untouched by #7715 (its prompts.py hunks stop well above api_skill_detail) and merge-tree --write-tree against refs/pull/7715/head is still clean with the fix in. A deferral whose only justification is "another round costs a push" is the failure mode this PR exists to end.

The mapping is the surface's own, not a new one. A refusal leaves content unset, which is the 404 an unreadable skill already produced, so nothing here is distinguishable from I/O trouble. One honest delta stated rather than left to be discovered: this branch had no size bound at all before, and the gate's default 50 MB ceiling now applies, so FileTooLargeError is caught and mapped to that same 404 instead of escaping as an unaudited 500 — a 50 MB SKILL.md is not a thing, and the previous behaviour was an unbounded read_text on the event loop.

Coverage was absent, so it is new. The package/ branch had no test of its own — _match_package_row is unit-tested but the handler branch was not — so test_a_hardlinked_package_skill_is_not_served and test_an_ordinary_package_skill_is_still_served are the first tests through it. Reverting the fix to Path(resolved).read_text() reds the first and leaves the second green, which is what separates the guard from the plumbing.

The lane's grep is now exhaustive for this surface, and I re-ran it: validate_file_path under src/kiro_crew/dashboard/ has no remaining validated-then-reopened content read. usage.py aggregates statistics and dispositions its own residual to #8079; core.py serves an operator-configured avatar, a different trust class. The one remaining instance of the class on this surface is site 4, which stays declared and deferred.

[bolichen97]

• Design Review 🟡 CONCERNS — the blanket st_nlink > 1 refusal covers package-shipped SOPs, where installers legitimately hardlink — rebutted

Installers that hardlink from a cache (uv's default link-mode on Linux, nix stores, dedup filesystems) leave st_nlink > 1 on every installed file, so on such installs every package SOP would list with an empty description and 500 on the unscoped read … Verify against a uv-Linux install, or scope the nlink refusal to the user-writable scopes where the planted-alias threat actually lives.

The mechanism is correct and I checked it rather than assuming — uv's default link-mode on Linux is hardlink, so a wheel's data files do land in site-packages with st_nlink == 2. What does not hold is the reachability, and it is unreachable by construction rather than by luck.

The package half of the prompt listing has no entries at all in this product. _list_aim_prompts walks only what PromptSourceProvider.prompt_source_roots() returns, and the public provider is DefaultPromptSourceProvider.prompt_source_roots() -> [] (src/kiro_crew/platform/defaults.py:303); find src -name '*.sop.md' is 0 files, so the wheel ships none for uv to hardlink in the first place. The package/ skill branch is the same shape: it is reached only when _capability_manager().available() is true, and the public DefaultCapabilityManager.available() returns False (defaults.py:333). Both halves are edition seams, and an edition that fills them supplies its own roots — which need not be inside site-packages at all.

For the halves that DO have entries here, the refusal is a consistency gain rather than a new cost, which is the load-bearing half of the rebuttal. _api_user_prompt_detail — the scoped read, pre-existing on main — already refuses st_nlink > 1 through this same gate in both user scopes. So before this PR a hardlinked user prompt was already unserveable by the scoped read, the update verb and the delete verb, while the listing still published its description and the unscoped read still returned its body. The listing was advertising a file the rest of the API will not open. Nothing legitimately hardlinks a ~/.kiro/prompts/*.md or a checked-out .kiro/prompts/*.md; no installer writes either.

Why not scope the refusal to the user-writable scopes anyway. It would mean threading each entry's source into _extract_sop_description, and one of its two call sites (handlers/__init__.py:602) is inside the exact hunk #7715 deletes — so the change lands as a conflict with the PR this one was split out of. That cost is real and the benefit is a regression that cannot occur in the shipped configuration. It is also the only refusal in the set that a within_root check cannot substitute for: a hardlink's opened real path IS the alias's own innocent path, inside the root, so st_nlink is the whole guard against the primary exploit. Weakening it per-scope would trade a reachable hole for an unreachable regression.

On the second bullet — record the true refusal cause in SEL metadata. Agreed in principle and not done, for a reason that is about the mechanism rather than the effort: the gate reads and judges through ONE descriptor and returns a bare None, so the cause is not available to the caller. Recovering it means re-stating the path after the refusal, which is another by-name look at attacker-influenced input — the exact pattern this PR exists to remove — and it would report on whatever the path means by then rather than on the inode that was actually refused. Reporting it honestly needs the gate itself to return a reason, i.e. a change to hooks.safe_read_file_bytes_nolink's contract shared with ~30 consumers, which is a wider change than this PR and wants its own review. The anti-oracle argument does constrain only the HTTP response, as the lane says; the blocker here is that the audit log has nothing truthful to write. Stated here rather than in the spec, deliberately: the spec documents behaviour and the behaviour is simply "the refusal is reported as the surface's existing unreadable outcome" — it is the gate's return type that would have to change for an audit line to say more, so the follow-up belongs to hooks.safe_read_file_bytes_nolink and its consumers, not to this paragraph. I am not pushing a docs-only sentence for it while four lanes are mid-run on this head.

[bolichen97]

• src/kiro_crew/dashboard/handlers/prompts.py:252 — descriptor-gate refusals are omitted from SEL (span=6f77c3fb4ba7) — fixed

if raw is None: return "" / if skill_bytes is not None:
Hardlinked prompt or package skill -> descriptor gate denies the read -> fallback hides the permission denial from the audit log.

Real, and the Design lane raised the same gap independently ("today a gate refusal is undiagnosable"), which is what settled it. The refusal shapes this PR chose are deliberately indistinguishable from I/O trouble in the HTTP response, and that argument holds only for the response: SEL is operator-side and not reachable through the endpoint. Without a line there, the primary exploit is not merely refused, it is invisible — an entry that lists with an empty description is byte-identical to a prompt that simply has none, and a 404 is what an uninstalled skill name produces. So the planted alias the PR exists to stop would leave the operator nothing to find.

All three refusals now write one through prompts._audit_unread. blocked when validate_file_path refused the name outright, error otherwise, and too_large for the skills read's FileTooLargeError; best-effort, so an audit write can never fail a listing or a detail view. The unscoped prompt read already logged its own outcome="error" and is unchanged.

The line records THAT bytes were withheld and never why, and that is the gate's constraint rather than a choice. safe_read_file_bytes_nolink judges and reads through ONE descriptor and answers a bare None, so a refused inode (hardlinked, non-regular, escaped its parent) and an ordinary read failure are the same value; recovering the cause means re-stating the path, which is another by-name look at exactly the input these reads stopped trusting, and it would report on whatever that path means by then rather than on the inode refused. blocked is carved out because it is the one cause that precedes any open. Reporting more honestly needs the gate itself to return a reason — a contract change shared with its other consumers, and a wider change than this PR.

Tests, revert-proven. test_a_refused_description_leaves_an_audit_line and test_a_sensitive_description_target_is_audited_as_blocked (test_prompts.py) and test_a_refused_package_skill_leaves_an_audit_line (test_skill_browser.py) all go red with the _audit_unread calls removed; each also asserts the ordinary neighbour produces no line, so the log cannot pass by saying everything. The pre-existing test_detail_unreadable now enumerates both lines by tool_name instead of counting all calls — two reads were genuinely withheld for a mode-000 prompt, so two lines are the honest count, and the assertion names each lane rather than loosening.

[bolichen97]

• src/kiro_crew/dashboard/handlers/prompts.py:2397 — the package-skill read blocks the event loop (span=6f77c3fb4ba7) — fixed

skill_bytes = safe_read_file_bytes_nolink(...)
Large or slow package skill -> GET handler -> synchronous descriptor read of up to 50 MB -> gateway tasks stall until completion.

Real, and it is this handler's own convention that decides it. api_skill_detail's DELETE and PUT verbs are already await asyncio.to_thread(...), and its kiro-user/ GET branch already goes to the discovery executor, each with a comment naming network-backed storage as the reason. No caller-supplied cap applies to the package/ read, so it reads up to the gate's own 50 MB default; the pre-fix read_text was unbounded, so the bound moved the right way, but neither belongs on a loop shared with every other session's turn against dashboard.loop_stall_exit_after_secs.

Now await asyncio.to_thread(safe_read_file_bytes_nolink, resolved, within_root=...). Nothing about the gate's guarantees moves with it: the open, the fstat, the st_nlink refusal and the fd-real-path check all still happen on one descriptor inside one call, so there is no new window — only the thread it runs on changes.

The two prompt-side reads are deliberately left on the loop, and the distinction is the bound, not consistency. api_prompt_detail passes max_bytes=MAX_PROMPT_BYTES (100 KB) and the listing's description read passes allow_truncate, so both read at most read_limit + 1 bytes. Moving a bounded 100 KB read off the loop buys a hop per request and no protection; the 50 MB one is the case that can actually outlast the watchdog budget.

Test, revert-proven. test_the_package_skill_read_runs_off_the_event_loop (test_skill_browser.py) wraps the gate, records the thread it runs on, and asserts the loop's own thread is not among them — a shape assertion, not a duration, so it cannot flake on a shared runner. Restoring the direct call reds it.

[bolichen97]

• First Principles 🟡 CONCERNS — the "four sites" census misses a fifth sibling: skills.py:2437 still reads a validated name by name — rebutted, and added to the census here

src/kiro_crew/skills.py:2433-2437, load_skill's _extra_paths branch. _extra_paths includes edition-contributed roots (skills.py:1696-1708) — the same "write access to a capability-seam package skill root" actor as fixed site 3 — and the two neighbouring branches in that file already read through the hardened descriptor reader (skills.py:2468). It belongs in the census, fixed or explicitly deferred.

The site is exactly where the lane says it is — I checked the file, not the label — and it is the fifth of this shape rather than a sixth. What does not hold is that its neighbours treat that class of root differently, and correcting that is what settles it. skills.py:2468 calls _read_enumerated_skill_bytes(skill_file, _within, ...), and that function's FIRST branch is if within is None: return path.read_bytes(). Its docstring names the members of that branch outright: "the global skills dir, extra paths, edition roots, and the paths writers construct themselves … these are operator-installed, so there is no directory to confine them to". So the neighbour is hardened only when a PROJECT grant supplies a root; handed an _extra_paths root it takes the same by-name read as line 2437. There is no inconsistency inside skills.py to repair — there is a documented decision, with named costs attached: taxing that branch with the hardened reader "measurably slowed the per-message listing path (test_skill_listing_cost guards it) and emptied frontmatter on Windows, which stopped anything looking pinned and dropped skill bodies out of the context entirely".

The actor is also not the same as fixed site 3, and that is the load-bearing half. Site 3's path arrives from CapabilityManager.list_skills() at request time. _extra_paths is resolved once at loader construction from cfg.skills.extra_paths — the operator's own config — plus mcp_tooling.extra_skills(). Nothing a checkout supplies reaches it, and the entry it serves is <extra>/<name>/SKILL.md where name has already passed _safe_name. Its immediate sibling one branch above (self._dir / name / "SKILL.md", line 2425) reads with no validate_file_path at all, because the operator's own skills directory is trusted; the _extra_paths branch is the STRICTER of the two already.

Applying this PR's gate there would trade an unreachable regression for a reachable one, which is the specific reason not to. An edition contributes roots that can sit inside site-packages, and a wheel installed by uv on Linux lands with st_nlink == 2 by default link-mode. A blanket st_nlink > 1 refusal on that branch would therefore empty legitimate, operator-installed skill bodies on ordinary installs — the same mechanism the Design lane flagged for the prompt sites, except here the reachability the Design finding lacked is present, because the root really can be an install directory. The prompt sites do not carry that risk: nothing installs a ~/.kiro/prompts/*.md or a checked-out .kiro/prompts/*.md, and the scoped read on main already refuses st_nlink > 1 in both user scopes, so this PR made the listing and the unscoped read agree with a refusal that was already there.

So: examined and deliberately left alone, per the brief's instruction that an unnecessary hardening change in a security-sensitive path is a cost rather than a win. Recorded here as the census entry the lane asked for. Any future change to that branch belongs with _read_enumerated_skill_bytes' within is None contract and its two named costs, not with this PR.

On the Subtractions note (the hardlink rationale ships five times). Fair, and I am not spending a push on prose while the lanes are mid-round on a security fix; the shared half is already in hooks.py's own docstring and in the new spec paragraph, and the per-site comments do each carry site-specific facts (which root, which refusal shape, which bound). Worth trimming in whatever round next touches this file.

[bolichen97]

• Design Review 🟡 CONCERNS — an edition that hardlink-installs its SOP / capability-skill roots would silently lose descriptions (site 1) and 404 (site 3) — rebutted, with the "verify" half of the lane's own ask done mechanically

But site 1 also describes edition-contributed *.sop.md roots (prompt_source_roots(), CPP seam) and site 3 reads CapabilityManager.list_skills() rows — both installed content by definition. … Dormant in the public fork (Default seam is []), so proceed — but verify or document the constraint that capability/SOP roots must not be hardlink-installed, or scope the st_nlink refusal for package-sourced rows.

The mechanism is right and I re-verified it on this head rather than restating the earlier round's answer. DefaultPromptSourceProvider.prompt_source_roots() returns [] (platform/defaults.py:303) and DefaultCapabilityManager.available() returns False (defaults.py:333), and the wheel ships zero *.sop.md files (find src -name '*.sop.md' → 0). Both halves the lane names are therefore EMPTY in the shipped configuration — not merely unlikely, but unreachable: _list_aim_prompts walks only what prompt_source_roots() hands it (handlers/__init__.py:553), and the package/ skill branch is entered only when _capability_manager().available() is true. There is no configuration of this repo in which a st_nlink > 1 refusal empties an SOP description or 404s a package skill.

On the residual for an edition that does fill those seams: the exposure is smaller than the finding reads, and the alternative is worse. A refused description keeps its listing entry — name, fullName, path and package are all still emitted; only description narrows to "", which is what an SOP with no heading and no frontmatter already produces. The audit volume is bounded by _prompt_cache / _PROMPT_CACHE_TTL, so it is one line per affected SOP per cache miss, not per request. And scoping the refusal to user-writable rows is not a free tightening: a hardlink's opened real path IS the alias's own innocent path, inside the root, so st_nlink is the ONLY signal the primary exploit leaves and within_root cannot substitute for it. Waiving it for source: "package" rows would restore the hole for exactly the roots an edition supplies — and it needs each entry's source threaded into _extract_sop_description, whose second call site (handlers/__init__.py:602) sits inside the hunk #7715 deletes.

Why the "document" half is not taken in this PR, stated as a knowingly out-of-scope sibling rather than skipped. The constraint the lane wants written down is a contract on the CPP seam — "a root contributed through PromptSourceProvider / CapabilityManager is read through the descriptor gate, so an inode with a second link is refused" — and its durable home is the provider docstrings in platform/interfaces.py, not learn-cron-dashboard.md, which documents the public dashboard surface where those seams are empty. Writing it into the dashboard spec would document a behaviour no configuration of this repo exhibits, in the file least likely to be read by whoever implements a provider. The lane's verdict is advisory and its own conclusion is "so proceed"; the verification above is the half of the ask that is answerable here, and I am not pushing a docs-only sentence into the wrong file while this head's shards are still running.

Rule candidate: when a security refusal keys on a property an INSTALLER can set (st_nlink, mode, ownership), check reachability at the seam that supplies the paths rather than at the read — a refusal that is unreachable because the provider returns [] needs no per-source waiver, and adding one re-opens the hole for precisely the paths the seam exists to contribute.

[bolichen97]

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 #7105 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 #7105: MERGE_DISCUSSION. The two changes are semantically compatible but occupy the same lines, and a naive conflict resolution silently reverts one side's security fix. The merge needs to be done deliberately by whoever lands second, not left to the diff. Files: src/kiro_crew/dashboard/handlers/prompts.py.
• PR #7715 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 #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.
• This PR is OVERLAPPING with PR #8259. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8249: MERGE_DISCUSSION. Write-side key policy on the same handler, materially different goal and code. Files: src/kiro_crew/dashboard/handlers/prompts.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

[bolichen97]

• First Principles 🟡 CONCERNS — the "five sites" census misses two more validate-then-reopen-by-name sites, never examined — accepted-and-deferred (fix(security): route the last two validate-then-reopen-by-name reads through the descriptor gate #8429)

The description's "five sites" table reads as the class's census, but the validate-then-reopen-by-name shape exists undispositioned at least twice more: _safe_read_snapshot (chat_runner.py:1234→1243, p.read_text after validate_file_path, bytes published into dashboard diff chips) and _read_spec_prefix (taskrunner.py:1298→218, bare open after validation). Accepted-and-deferred is fine — but these two were never examined.

The lane's actual ask here is that these be examined instead of silently omitted, and that is the part worth answering directly: both were examined, and both reproduce the shape. _safe_read_snapshot in src/kiro_crew/dashboard/chat_runner.py calls validate_file_path(path) and then re-opens the validated name with Path(validated).read_text(...) at :1243, publishing those bytes into the dashboard's diff chips. taskrunner.py validates at :1298 and _read_spec_prefix then does a bare open(path) at :218. In both, validate_file_path judges the name while a second, unverified open produces the bytes, so a hardlink alias defeats them exactly as it defeated the three sites fixed here — realpath yields the alias's own innocent path and is_symlink() is False, while the inode belongs to whatever it aliases.

Deferred rather than folded in because the correction is real but belongs to a different surface. This PR's census is scoped to the prompt/skill endpoints named in its title, and both new sites live in other modules with their own callers and their own failure contracts — the snapshot read returns None, the taskrunner has its own unreadable-spec behaviour, and each has to keep that shape so no caller becomes an oracle. Widening here would take the diff into two modules the PR does not otherwise touch and re-arm every lane on work whose correctness has to be argued separately anyway.

Tracked in #8429, which names both call sites with the surrounding code, prescribes the same hooks.safe_read_file_bytes_nolink gate used here (open first with O_NOFOLLOW, then fstat that one descriptor for st_nlink > 1, a non-regular inode, and an is_sensitive_path target, with each canonical parent passed as within_root so the guarantee also holds on Windows), and asks for a planted-hardlink test mirroring the ones added to test/test_prompts.py here. I have taken the census claim in this PR's description to mean the prompt/skill surface it fixes, which is what it enumerates; #8429 is the rest of the class.

[bolichen97]

• First Principles 🟡 CONCERNS — the audit-on-refusal invariant has 2 unfixed siblings in the steering surface this PR cites as precedent — accepted-and-deferred (fix(dashboard): audit steering's two silent descriptor-gate refusals #8430)

The audit-on-refusal invariant item 4 introduces ("a silent fallback is what makes a planted alias invisible" — _audit_unread's own docstring) has 2 unfixed siblings in the surface this PR cites as precedent: steering's listing read returns _empty_meta() (steering.py:242) and its scoped read returns readfailed (steering.py:618) on the same gate refusal, with no SEL line — grep log_tool_invocation in steering.py: 2 hits (:995, :1157), neither on a refusal path.

Verified rather than taken on trust, and the lane is right on every particular. In src/kiro_crew/dashboard/handlers/steering.py the listing description read swallows a refusal into _empty_meta() at :241 and :243, the scoped read returns "", display, "readfailed" at :618, and log_tool_invocation appears only at :995 and :1157 — neither on a refusal path. A refused steering read therefore leaves no operator-visible trace at all.

Deferred rather than fixed here because of what the two surfaces actually differ in. The SEL line is a rider on this PR, not its fix; the fix is the three prompt/skill reads that were serving unverified bytes. The steering sites are not serving anything — the gate already refuses correctly there, so what is missing is observability, not containment. That is the distinction that makes this deferrable at all: no bytes escape either site today, so nothing is shipping broken while it waits. Extending a newly-introduced invariant into a second handler module this PR does not otherwise touch would widen the diff into unrelated code and re-arm the full review wave for a change with no behavioural effect on the defect this PR closes.

Tracked in #8430 with the two invariants that have to survive the port: record only THAT bytes were withheld and never why, since re-statting the path to recover a cause would be another by-name look at the input these reads exist to stop trusting; and leave _empty_meta() / readfailed unchanged so the HTTP answer stays indistinguishable from I/O trouble and no endpoint becomes an oracle for whether a path is protected. The issue also raises whether the wrapper should become a shared helper rather than a third module-local copy.

[bolichen97]

• First Principles Subtraction — collapse the three near-verbatim copies of the Windows/within_root rationale to one docstring plus pointers — rebutted as disproportional to act on now, not as wrong

Shrink the twice-repeated ~25-line Windows/within_root rationale: keep it in _extract_sop_description's docstring (prompts.py:230-258) and reduce the near-verbatim comment blocks in api_prompt_detail (prompts.py:392-417) and api_skill_detail (prompts.py:2419-2444) to one-line pointers — four divergent copies (plus the spec paragraph) of one argument.

The observation is accurate and I am not disputing it. The three regions are 29, 26 and 26 lines, and while the third opens by back-referencing ("Same descriptor gate as the prompt reads above, for the same reason:") it then re-derives the whole argument anyway, so "divergent copies" is a fair description of what a future editor would have to keep in step.

What makes it disproportional is the cost of acting on it at this moment, not the merit. This is a comment-only edit with no behavioural effect, and the PR is currently converged: readiness success, zero red checks across the 60-check wave, and GPT 5.6, Opus 4.8 and Design Review all green with freshness stamps on this exact head. Amending to reflow three comment blocks discards all of that — it re-arms every lane and the full wave for roughly forty minutes, and this repo has a documented precedent (#4070) of a one-line behaviour-preserving edit made in response to one reviewer producing a new blocking finding in a span an earlier round had passed clean. Spending a converged green state to consolidate prose is the wrong trade, particularly for an item the lane itself files under Subtractions with an advisory CONCERNS rather than a BLOCK.

The rule I am applying is the narrow one: this is about not doing work this PR does not need, and it is explicitly not a claim that the fix is incomplete — no reachable behaviour, no guard and no test is affected either way. So the commitment rather than a refusal: if any further push to this branch becomes necessary for a real reason — a CI red, a blocking finding, or a defect in this diff — I will fold this consolidation into that same amend, where it costs nothing extra. If the branch reaches merge without another push, the duplication ships as three comments that agree with each other, which is a readability cost and not a correctness one.

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.