feat(aws-control): read an external prompt reference safely

[chenmingwei23]

Stacked on #9213. Merge that first; this PR's diff is only the external prompt reader.

Why it is separate

#9213 refuses an agent spec whose prompt is a file:// reference. This adds the fences that make reading one safe.

The split is not about line count. Across seven review rounds on #9213, this ~350-line region produced 20+ separate blocking findings while the rest of the module was settled, and several fixes to it created the next round's finding. Reviewed alone, a reader can hold all of it at once.

One test file is deleted, deliberately

tests/test_external_prompt_refused_for_now.py is removed (-88 lines). It pinned the base
branch's REFUSAL of a file:// prompt, which is the behaviour this PR exists to lift, so it
cannot pass and this change land at the same time. Its replacement is
tests/test_external_prompt_supported.py, which drives the same shapes through the reader:
a persona inside the agents directory, an absolute persona outside it, a symlinked persona,
and a swapped parent, the last of these end to end in test_a_swapped_parent_INSIDE_the_agents_dir_is_still_refused, which renames the real subdirectory aside, links its name at a secrets directory and asserts both the refusal and that no key material reached agent.json.

Stated here because a deleted test named "refused for now" reads like a silent revert to
anyone who has not traced it. The no-follow read path is not weakened by this PR: counted
across the packaging tree, base to head, _read_text_nofollow goes 17 to 38,
_read_text_openat 13 to 18, _NOFOLLOW_READ_FLAGS 10 to 13, _is_redirecting_entry 31 to
33, and _walk_no_reparse, scandir, and both credential detectors are unchanged. No symbol
in that tree drops.

What is here

Five functions, and the reason each exists:

_resolve_prompt_path	judges the chain BEFORE any resolution, because Path.resolve() IS the traversal
_open_nofollow_under	opens each component relative to the previous descriptor with O_NOFOLLOW
_open_root_nofollow	opens the anchor from ITS parent, so the anchor itself is not swappable
_open_attr_checked_under	the lstat walk where dir_fd is unavailable
_read_text_nofollow	reads through the descriptor the checks were made against

Two properties are worth stating because they are easy to get wrong:

resolve() collapses links, so a redirect check placed after it can never find one. An earlier version of this code walked the resolved path looking for reparse points and could not have worked; it passed its own tests only because they called it directly with an unresolved path.

A Windows junction is a reparse point that is NOT reported as a symlink, and shutil.rmtree traverses one rather than unlinking it. Entries are judged by attribute, not by is_symlink().

Where dir_fd is unavailable the walk is by lstat, which catches a redirect planted before the build completely and a live swap not at all. That is weaker than the descriptor walk and is documented as weaker rather than presented as equivalent.

Verification

228 packaging tests pass. Every fence has a test that reddens when the fence is reverted, verified by mutation. The Windows-only branches are pinned by source rules, because no test on a POSIX host reaches them.

file:// is used by shipped agents (apps/builtins/pptx_maker/agents/*.json), so those crews are bundleable once this lands.

Declared: everything in this diff that is not the prompt inline

Six review items asked for these to be justified or removed. All six are actioned.

Removed a local opener stack. _open_nofollow_under, _open_root_nofollow and
_open_attr_checked_under (191 lines) had zero production callers and duplicated
kiro_crew/hooks.py. Counted outside tests they were 2 occurrences each, which is a
definition plus one internal call; the real prompt read goes through
hooks.safe_read_file_bytes_nolink. The root and what parameters went with them:
neither reader ever interpolated what, and the only caller that wanted an anchored walk
disappeared when the read was routed to hooks. Fifteen tests that pinned that mechanism
were removed with it, because a test of an unreachable guard reports coverage it does not
have.

Scoped the byte ceiling to the prompt read. It had been applied inside both shared
readers, which also serve the skill scan, the plan read and the agent-spec read, so a limit
named for prompts could refuse an oversized agent spec. The base bounds none of those. The
prompt read keeps its bound as max_bytes on the shared guard.

Restored nine POSIX-only markers in test_sensitive_source_and_report_identity.py,
dropped when this branch took its own side of that file during a merge. Without them those
nine execute on the Windows shards, where they can only produce reds unrelated to this
change.

Two 0o755 sites annotated with nosemgrep and a reason rather than lowered. They
restore a mode the fixture cleared to 0o000, and traverse permission is what the
temp-directory teardown needs.

One docstring edited outside my functions. _dir_fd_supported named
_open_nofollow_under as an example; that name no longer exists, so the clause now names
the three live call sites. The count it states, three, is unchanged.

Two strings this branch removes from _inline_prompt are the base's refusal to follow
a file:// reference and its matching docstring. Removing that refusal is the feature.
Measured across the whole file, no function outside the eleven this branch owns loses a
message.

A NUL in the reference is refused. file://per\x00sona.md reached a syscall as a bare
ValueError on all three branches (relative, absolute, and a NUL alone) and surfaced as a
traceback naming neither the spec nor the reference. Checked on the string, because Path
accepts a NUL and defers the error past every point that could still name it.

External-input audit of this commit (issue #9575)

GPT flagged build.py:716 as an unhandled anchor resolution. That is instance six of one
gap, filed as #9575: an operation on author-supplied input called without deciding what its
failure means. Rather than patch the one line, every such call in this commit was audited by
walking the AST of the nine functions it adds or changes and recording, per call, what the
failure becomes. Consistent with #9575's proposal, absence and unreadability are separated by
MESSAGE; the type-level question that issue leaves open is not settled here.

call	site	failure becomes
agents_dir.resolve()	_resolve_prompt_path	ExportRefused, names the agents directory
(agents_dir / target).resolve()	_resolve_prompt_path	ExportRefused, names the reference
path.resolve()	_resolve_prompt_path	ExportRefused, symlink loop
os.readlink(hop)	_resolve_prompt_path	EINVAL/ENOENT end the walk; anything else refuses
agents_dir.resolve()	_inline_prompt	ExportRefused, names the agents directory
os.lstat(path)	read_agent_spec	absent, uninspectable and unreadable refuse separately
os.open / os.fdopen ×6	_read_text_nofollow, _read_text_openat	None, and each caller words its own refusal

Three of these were unstated before this push and are the substance of it:

os.readlink collapsed "not a link" with "exists and could not be read". The first is
EINVAL and ends the walk normally. The second ending the walk lets the resolve() below
traverse a hop nothing judged, which on Windows is the SMB probe the walk exists to prevent.

os.lstat on the agent spec reported a permission error as "no agent spec ... nothing to
deploy", sending the operator after a missing file while the spec sits there unreadable. All
three outcomes now refuse where they are detected rather than through a sentinel the next
branch re-reads, which is also the shape #9575's completeness test could check.

agents_dir.resolve() raised RuntimeError on a link cycle, uncaught, on both the relative
and the absolute path in. Measured: a two-link cycle at agents/ produced
RuntimeError: Symlink loop out of the CLI.

The six os.open / os.fdopen calls return None deliberately, which is this module's
existing contract: the reader cannot know whether the caller wants "agent spec" or "curation
plan" in the message, so each caller words its own refusal. That collapses absence and
unreadability at the reader, and the callers separate them again.

Each of the three has a test that reddens when the guard is removed. The hop test injects the
readlink failure and says so: reaching it needs a hop whose lstat succeeds while its
readlink fails, and on POSIX both need the same parent traverse permission.

Author-supplied path operations: window coverage and failure meaning

Four consecutive rounds on this seam each fixed the line that was named. Both rules that
produced those findings apply to every operation in this commit, so the whole set is
enumerated here rather than sampled. The set is derived mechanically: every function whose
source differs from the base, then every filesystem call inside them (9 functions, 21 calls).
Two questions per row.

Window says whether the guard covers the interval between the check and the use, or
whether something is re-derived from a name in between. Failure says what the operation's
failure becomes.

Line	Function	Operation	Window	Failure
691	_within	path.relative_to(root)	no filesystem access, pure path arithmetic	ValueError returns False, which is the question asked
722	_resolve_prompt_path	agents_dir.resolve()	this is the resolution; the chain walk runs before it, pinned by a test on call order	OSError/RuntimeError to ExportRefused; this libc reports link cycles as RuntimeError
787	_resolve_prompt_path	(agents_dir / target).resolve()	same	same
794	_resolve_prompt_path	path.relative_to(agents_root)	pure	ValueError to ExportRefused
827	_resolve_prompt_path	path.relative_to(path.anchor)	pure	cannot fail: total against the path's own anchor, measured on absolute, relative and empty shapes
844	_resolve_prompt_path	os.readlink(hop)	judges the name before resolution, which is the point of the walk	EINVAL/ENOENT end the walk, anything else refuses
903	_resolve_prompt_path	path.resolve()	after the chain walk	OSError/RuntimeError to ExportRefused
991	_read_text_nofollow	os.open(path, O_NOFOLLOW)	opens by name, then judges the descriptor it got	OSError returns None; each caller words its own refusal
995	_read_text_nofollow	os.fstat(fd)	descriptor, so the inode judged is the inode read	OSError returns None
1065	_read_text_openat	os.open(root)	opens the walk's root	OSError returns None
1071	_read_text_openat	os.open(part, dir_fd=cur_fd)	descriptor-relative per component	OSError returns None
1074	_read_text_openat	os.open(leaf, dir_fd=cur_fd)	descriptor-relative	OSError returns None
2060	read_agent_spec	path.relative_to(anchor)	pure; anchor is derived from path, so it is an ancestor by construction	cannot fail, for that reason
2067	read_agent_spec	os.lstat(path)	classifies why the read failed, authorises nothing	absent, uninspectable and unreadable each refuse where detected
2486	_inline_prompt	agents_dir.resolve()	before the pin is taken	OSError/RuntimeError to ExportRefused
2558	_inline_prompt	os.open(anchor, O_NOFOLLOW|O_DIRECTORY)	opens the pin, held to 2632 across every step below	OSError to ExportRefused
2570	_inline_prompt	path.relative_to(anchor)	pure	guarded by _within in the same expression, so the raising case is unreachable
2592	_inline_prompt	safe_read_file_bytes_nolink(path, anchor)	not covered by this call: it resolves both names itself. Authorised separately at 2640 against an identity taken through the pin, and a byte disagreement refuses	FileTooLargeError to ExportRefused; None to ExportRefused
2607	_inline_prompt	path.relative_to(anchor)	pure	guarded by _within in the same expression
2609	_inline_prompt	os.stat(rel, dir_fd=anchor_fd)	descriptor-relative, so it names the file inside the directory that was checked	OSError to ExportRefused
2640	_inline_prompt	safe_read_file_bytes_with_identity(path, {inode})	opens by name, but the allowlisted inode decides, so a substituted name cannot be read	PermissionError (identity), then OSError (the right file, read part way), FileTooLargeError and None each to ExportRefused. The specific arm is first because it subclasses the broad one

One row needed a fix this round. 2609's observation authorises an inode, and it did not say
what kind of inode: a persona replaced by a directory between the two reads had a directory's
inode allowlisted, and the read then failed inside the shared reader as an uncaught
IsADirectoryError, out of a function whose contract is ExportRefused. Reproduced, then a
regular-file and a hard-link verdict added on the pinned stat, before the identity is handed
over, because nothing after the allowlist can refuse it.

Each verdict reddens only its own case, so neither masks the other:

regular-file verdict removed   directory=FAILED  hard_link=passed
hard-link verdict removed      directory=passed  hard_link=FAILED

The two halves of the anchor pin are also independent. Widening the identity allowlist reddens
both swap shapes; disabling the byte comparison reddens only swap-and-restore, which is the
shape that defeats comparing the anchor's identity before and after.

identity allowlist widened   swap=FAILED   swap_and_restore=FAILED
byte comparison disabled     swap=passed   swap_and_restore=FAILED

The table's method was wrong, and one row was wrong with it

The row for 2640 listed FileTooLargeError, PermissionError and None. Those were the three
handlers written at that call, not the failures the call can produce: the reader also raises a
plain OSError when the read of the correct file fails part way, and that left a function
contracted to raise ExportRefused as a bare traceback. GPT found it; the table should have.

The method is what produced it. A row that enumerates the handlers present cannot discover a
missing one, because the thing it reads is the thing under question. Re-auditing every row by
what each CALL can raise, established by reading the callee rather than the handler, finds:

• the identity read, now fixed and covered by a test that reddens when the arm is removed;
• three os.close calls in finally blocks that convert nothing. Left as they are, because a
  close that fails means the descriptor was already invalid and swallowing it would hide that,
  and because the two readers this file has always closed that way. Stated rather than silent;
• nothing else. The five relative_to rows that answer "cannot fail" were measured, not
  assumed: total against a path's own anchor, or guarded by _within in the same expression.

One row is right for a reason the table did not give. The sibling read at 2592 states "None to
ExportRefused" and that is complete, but not because the same reasoning applies: in hooks,
safe_read_file_bytes_nolink catches OSError itself and answers None, while
safe_read_file_bytes_with_identity wraps its read in a try whose only clause is finally.
Two readers, two different failure surfaces, and the table had treated them as one.

Cheapest guard first

_refuse_share_reached_through_ancestors walks a hop's ancestors with lstat. On Windows that
touch IS the probe, so a walk beginning at a share-shaped anchor performs the exchange it is
looking for. The string test now runs in front of it: characters are read, nothing is reached,
and only a path that survives earns a filesystem question. Pinned by a test that records every
filesystem question the walk asks and requires a share-shaped hop to produce none.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fix to a deliberately deferred problem, but the pinned-anchor read protocol is a one-off security composition that will face immediate duplication pressure.

Watch

• The safe-read protocol in _inline_prompt (shared reader → anchor-fd stat → local S_ISREG/st_nlink verdicts → second identity-pinned read → byte equality) is a bespoke TOCTOU consensus composed at one call site in one builtin's bundle builder. The PR's own rationale for deleting the local opener stack — "a second copy that agrees today is a second copy that drifts tomorrow" — applies to this composition too, and the same pptx_maker specs carry more file:// references (skill/workflow arrays) that any future inlining will need this exact dance for, re-derived by hand each time.
  Clears when: the anchor-pinned read is promoted into kiro_crew.hooks as one primitive (a descriptor-relative read owned where the other read rules live), or a maintainer explicitly accepts the call-site composition as final.
• The new nt-scoped fencing (_refuse_share_reached_through_ancestors, the readlink hop walk, the UNC gate in _resolve_prompt_path) is unreachable today — the builder refuses at entry on platforms without the no-follow primitive — and that guard "lifts automatically when the primitive is available." Code never executed on real Windows (tests are POSIX-marked or monkeypatched) would become the live SMB/NTLM boundary via an environment probe rather than a human decision.
  Clears when: the entry guard's lift becomes an explicit enablement (issue #9496's exit requires a decision, not a probe), or the nt branches gain real-Windows CI before the guard can lift.

[DESIGN-REVIEWED] 1b7b05e

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 1b7b05e3da01c7be1942ebb013045ff209810b74 — 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 verifications are done. The shared hooks symbols exist, the pptx_maker agents genuinely carry file:// prompts (the derived requirement), the deleted test's own docstring called the refusal temporary, and all three build entry points call _refuse_without_nofollow_primitive() before any prompt handling — which makes the new nt-scoped branches production-unreachable today. Emitting the review.

First-Principles-Verdict: CONCERNS

Two riders: a test pinning comment prose in two unrelated modules, and "annotated rather than lowered" chmods that the diff actually raises 0o700→0o755.

Not justified as shipped

• 5. Windows-only UNC fence branches — zero consumers: all three build entry points call _refuse_without_nofollow_primitive() first (build.py:2341, 2604, 3840), so no production path reaches them on the only platform they run on.
• 8. Teardown chmods — rides along, framing mismatch: description says "annotated … rather than lowered", but base had 0o700 and the diff raises it to 0o755; owner traverse (0o700) already satisfies the stated teardown need.
• 9. test_the_windows_narrowing_is_the_repos_own_settled_answer — rides along, oversized: asserts comment-prose fragments of pinned_fs.py and eval/bench/safepath.py from a packaging suite; a rewording there reds this test.

What this change ships

Inventory (10 items) — 7 justified

Intent: let an operator bundle a crew whose agent spec references its prompt as file://, by reading the persona safely and inlining it — an ADDITION (lifting a pinned refusal).

1. A file:// prompt is now read and inlined instead of refused — justified
2. The base's refusal pin test_external_prompt_refused_for_now.py deleted — justified
3. New refusals fencing the read (NUL, UNC, redirect chains, pseudo-fs, credential paths, 1 MiB ceiling, swap detection) — justified
4. External prompt refused outright where kiro_crew.hooks/security are unimportable — justified
5. Windows-only UNC fence branches — zero consumers (entry guard refuses Windows first)
6. Absent vs unreadable agent spec now refuse with distinct messages — justified
7. Shared readers read bytes then decode; text-call floor test loosened 4→3 with reason — justified
8. Two test teardown chmods widened 0o700→0o755 — rides along (framing mismatch, see above)
9. Source-rule test pinning pinned_fs.py/safepath.py prose — rides along, oversized
10. Error rewordings ("no longer exists" → "does not exist", ×2) — rides along, harm-free

Watch

• The ~150 nt-scoped lines (os.name == "nt" gate, absolute-branch readlink walk, _refuse_share_reached_through_ancestors) are dead in production: counted, every entry point (read_agent_spec, plan read, build_bundle) refuses Windows via _refuse_without_nofollow_primitive() before them. The description says only "no test on a POSIX host reaches them", not that no production path can.
  Clears when: the author states these are pre-work for lifting the POSIX-only gate, or deletes them until that lift.
• Item 1's claim "those crews are bundleable once this lands": the pptx_maker prompts are file://{ENGINE_ROOT}/… template forms; nothing in this diff expands {ENGINE_ROOT}, so _resolve_prompt_path would judge the literal braces.
  Clears when: one pptx_maker crew is bundled end-to-end, or the claim is scoped to post-expansion specs.

Subtractions

• In test_the_windows_narrowing_is_the_repos_own_settled_answer, delete the if settled.exists(): block (two asserts on safepath.py docstring fragments); the two machine-checkable pinned_fs predicate asserts carry the pin alone.
• Restore 0o700 at the two teardown chmods in test_sensitive_source_and_report_identity.py — owner traverse is all teardown needs, and it is what base shipped.

[FIRST-PRINCIPLES-REVIEWED] 1b7b05e

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/builtins/aws_control/crew/packaging/build.py:954 -- “SUPPORTED” contradicts _inline_prompt, which rejects absolute symlink personas; line 1100 likewise claims only external references fail when the shared fence is unavailable, but relative references also fail -> Fix: align these comments with the implemented refusal scope.
[GPT-REVIEWED] 1b7b05e

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've verified the mechanism against the source. Let me confirm my analysis of the finding.

The candidate is grounded:

(a) Input: A crew whose agent.json prompt is file:///abs/personas/frontdesk.md — a real regular file outside agents_dir (the documented supported case) whose path has a symlinked ancestor (macOS /var→/private/var, /tmp→/private/tmp; or a symlinked $HOME/mount on Linux — the design comment at build.py:2905-2909 itself calls symlinked ancestors "most real installs").

(b) Call path: _inline_prompt → _resolve_prompt_path returns the unresolved absolute path (build.py:924, path = Path(target); return path at 1128; the resolved local is never returned) → _within(path, agents_root) False → anchor = path.parent (unresolved, build.py:2926) → _open_dir_nofollow_pinned(anchor, already_resolved=True) (build.py:2989) → already_resolved=True skips .resolve() (build.py:1766) → walks every component from / with O_NOFOLLOW (build.py:1768-1772) → os.open on the symlinked ancestor fails ELOOP → caught at 2990 → ExportRefused.

(c) Outcome: The whole bundle build aborts ("the prompt anchor … could not be opened") for a documented supported case — exactly the "final-component check only" behavior the design comment at 2905-2909 promises the outside branch, and exactly the "Walking from / with O_NOFOLLOW would refuse any legitimate path whose ancestors include a symlink" failure it says must be avoided. already_resolved=True is correct only for the inside branch (anchor = agents_root, which is resolved); the outside branch hands it an unresolved parent, so the claim is false and the "resolve first" contract in _open_dir_nofollow_pinned's own docstring (1746-1753) is violated.

This is an over-refusal (functional/availability), not a security/crash/data-loss class — so advisory FINDING, fixable within the changed lines.

A supported absolute-persona-outside-agents_dir prompt is falsely refused when any ancestor of its path is a symlink.

FINDING — src/kiro_crew/apps/builtins/aws_control/crew/packaging/build.py:2989 — _open_dir_nofollow_pinned(anchor, already_resolved=True) on the outside branch, where anchor = path.parent is the unresolved absolute path (build.py:2926), walks every ancestor from / with O_NOFOLLOW and refuses at the first symlinked ancestor (macOS /var, /tmp; symlinked $HOME), aborting the bundle for the documented supported case and contradicting the "final-component check only" design at build.py:2905-2909 → Fix: for the outside branch resolve the parent once before pinning — anchor = path.parent.resolve() (keeping already_resolved=True), or pass already_resolved=False so the walk runs over the collapsed path as _open_dir_nofollow_pinned's own "resolve first" contract requires.

[OPUS-REVIEWED] 1b7b05e

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

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

[chenmingwei23]

Rebuttal: pinned traversal on Windows, _open_attr_checked_under

OUT OF SCOPE, citing this repository's own settled answer rather than disputing the analysis. The narrowing is real and is stated at the call site; what is disputed is that the requested fix is available here.

Three facts, each checkable in this tree:

1. The repo's own pinning module does not offer it either. pinned_fs.supports_pinned_walk() requires O_DIRECTORY, O_NOFOLLOW and os.open in os.supports_dir_fd, and returns False on Windows. Adopting pinned_fs here would not close this gap, so "use the shared helper" is not the fix.

2. Every caller in the tree branches on that predicate rather than assuming it. apps/routes.py (two sites), apps/manager.py, dashboard/handlers/steering.py, dashboard/handlers/files.py, dashboard/handlers/prompts.py. The repository's shape for this platform is a documented narrowing, not an anchored walk.

3. A ctypes no-reparse handle was considered and rejected in-repo. eval/bench/safepath.py reaches this exact question and settles it against a CreateFileW with FILE_FLAG_OPEN_REPARSE_POINT, because it buys a property another mechanism already gives "at the price of security code that cannot be exercised on the machine this harness is developed on". That reasoning applies unchanged here: no test on this host can plant a junction, so the code would ship unexercised.

What the Windows branch does instead: checks each component by attribute before any resolution, refuses a redirect planted before the build ran (the realistic shape -- a junction in a crew directory an operator is about to package), confirms after the open that (st_dev, st_ino) match what was checked, and states in the source that a swap inside the remaining window still wins. A live swap needs a concurrent writer on the operator's own machine mid-build.

All three facts are pinned by tests/test_sensitive_source_and_report_identity.py::test_the_windows_narrowing_is_the_repos_own_settled_answer, which fails if supports_pinned_walk stops gating on dir_fd, if pinned_fs grows a no-reparse path, or if the safepath.py precedent disappears -- so this rebuttal expires automatically when its premises do. test_the_windows_branch_says_what_it_cannot_do fails if the limitation stops being stated.