fix(code-review-sage): run the review worker under the resolved interpreter

[chenmingwei23]

Problem / Motivation

Code Review Sage does not run on Windows. Its review worker is handed
python3 sage_lib/... commands, and python3 is not an interpreter there:
neither the python.org installer nor python -m venv creates a python3.exe,
and the name resolves instead to a Microsoft Store app-execution alias that runs
no Python. The command produces no result record, so a review starts and ends
with no verdict — a silent failure. Rather than ship that, the app refused on
Windows, which was the last reason Sage did not work there while Issue Radar
(after #4613) did.

Why it matters

Windows users get no code review from the app at all. The refusal is the good
outcome of the two available today: without it, the review appears to run and
silently yields nothing, which is worse than a clear message. Removing the
refusal is the acceptance criterion for #4630, and it is only safe once the
worker names an interpreter that exists on the host.

What changed (motivation → approach → change)

Symptom: the worker's Python commands never run on Windows. Root cause: the
prompts and the shipped skills name a platform-specific alias instead of an
interpreter. So the change stops naming an alias.

The five prompt sites in sage_lib/review_driver.py now name the gateway's
own sys.executable — an absolute path to a real interpreter on every platform.

This is deliberately not the shared resolve_app_python(app_root) policy,
which prefers <app_root>/.venv. store.app_root() resolves under
KIROCREW_HOME, the same writable tree the review worker writes into, and that
worker is prompt-injectable: a planted .venv/Scripts/python.exe would be
executed by the next review, which is arbitrary code execution as the gateway
plus forged result records — persistence into a later run rather than a
capability the worker already had. Nothing is given up by declining the venv
preference here: that preference exists so an app's own dependencies are
importable, this app ships no requirements.txt, and the only non-stdlib import
anywhere in sage_lib/ is kiro_crew itself, which is importable under
sys.executable by construction. An app venv would in fact be the weaker choice,
since it need not have kiro_crew installed at all.

The path is handed over raw, with no shell quoting. Quoting requires knowing
the worker's shell, which is not pinned: a Windows session may get PowerShell or
cmd, and a form valid in one is a syntax error in the other (a quoted path needs
PowerShell's call operator, which cmd rejects). Since a Windows profile with a
space in it is ordinary rather than exceptional, guessing wrong would restore the
same silent no-result failure for a large share of Windows users. The prompt
therefore tells the worker to quote as its own shell requires.

The two shipped skills carry commands too, and app skills are registered by
symlink/junction into ~/.kiro/crew/skills rather than copied
(apps/bridges.py), so there is no rendered file to substitute a placeholder
into at load time. They therefore stop naming an interpreter at all: commands
read <python> …, and each skill opens with a note pointing at the one the task
prompt names. learn-from-sage/SKILL.md is included because the review prompt
routes inline miss-analysis through it, so its commands reach the worker exactly
as the review skill's do.

The refusal in sage_lib/discovery.py is removed, which is what closes the
issue.

Removing it makes a second Windows gap live, so this closes it too. Every
record, report and cache the app stages was locked to its owner with
os.chmod(0o600), which expresses that only on POSIX: on Windows it toggles the
read-only attribute, leaves the inherited DACL intact, and succeeds — so the
files would stay readable by every other local account with nothing raised to
notice. The sage_lib call sites now route through one seam,
store.open_locked_temp, which creates the temp with mkstemp and applies
platform_compat.restrict_to_owner before returning the descriptor; the
POSIX path is the same chmod, including the fail-loud OSError the callers' temp
cleanup relies on.

Two further changes in backend/routes.py follow from that one and are called
out because they are behaviour changes, not refactors.

• The temp files are no longer predictably named. Both writers (the run
  registry and the settings writer) previously used <name>.tmp and a bare
  os.replace. A predictable name in a worker-writable directory lets a
  prompt-injected worker pre-plant a symlink there, and Path.touch() follows
  one — so the lockdown and the payload would both land on an arbitrary
  user-owned file. Both now call kiro_crew.atomic_write.atomic_write(..., restrict_to_owner=True), which picks an unpredictable mkstemp name, locks the
  temp down before the payload, refuses a planted parent link, and replaces
  through replace_with_retry — closing a transient-sharing-violation gap on
  Windows that the bare os.replace had (the class tracked in deploy pending confirmations can fail on a transient Windows replace sharing violation #4701 / md-notebook: a contended rename loses the note save on Windows #4898).
• The registry save is now offloaded. restrict_to_owner spawns icacls on
  Windows, so the write can no longer run on the gateway's event loop. _save_runs
  is now a coroutine that serializes the registry on the loop — preserving the
  snapshot-under-the-caller's-lock property — and offloads only the file write via
  asyncio.to_thread; all eight call sites await it.

The test suite stays gated on Windows and its conftest now says why: the
harness is POSIX-only (0600 as st_mode bits, forward-slash suffixes,
shell-script gh stubs), which is separate work from making the app run. The
docstring previously gave two reasons; one is now false, so only the true one
remains.

Tests

• The existing worker-prompt guard asserted the prompts contain a
  python3 <path> reference, so it failed on this change rather than
  passing vacuously. It is now interpreter-agnostic, still checking every
  referenced script path exists.
• TestWorkerPromptInterpreter — no prompt names a bare interpreter; every
  script command carries the resolved one; the prompt states the interpreter for
  the skills' <python> placeholder; the resolved value is absolute; and
  test_the_interpreter_is_never_taken_from_a_worker_writable_path patches
  store.app_root to raise, so any reintroduction of the app-root lookup fails
  the suite.
• TestInterpreterIsHandedOverRaw — a path containing a space, a $, or a
  backslash run arrives verbatim. Each of those was a separate defect in an
  earlier revision of this change, so the guard pins all three at once.
• TestShippedSkillsNameNoBareInterpreter — reads the shipped SKILL.md files
  and fails on a bare-interpreter command. A python3 left in a skill reaches
  the worker exactly as a prompt string would, and the prompt guard cannot see
  it.
• TestRestrictToOwner — the lockdown delegates to the runtime helper (not a raw
  chmod, which succeeds while doing nothing on Windows), a lockdown failure
  propagates so the callers' temp-file cleanup still runs, owner-only permissions
  are applied on POSIX, and a failed lockdown leaves behind neither the
  descriptor nor the temp file.
• Registry-write guards in test_backend_routes.py — the owner-only lockdown
  never runs on the event loop (the test records the executing thread id and
  compares after the call, rather than asserting inside a swallowed except);
  a planted runs.json.tmp symlink is not followed; and the predictable
  <name>.tmp path is never written, which is the symlink-free twin so the
  invariant is also covered on Windows, where creating a symlink needs a
  privilege CI does not grant.

Manual verification

Verified on a Windows 11 host that python_command() returns an absolute
interpreter and that all four commands in the built prompt carry it.

Sage's own suite was run on Windows with the collection gate lifted, against a
baseline captured at the same commit: 750 passed / 21 failed on this branch
vs 735 / 21 on the base. The 21 are the same pre-existing symlink-plant
tests, which need a privilege this account does not hold (WinError 1314) — so
this change adds passing tests and no failures. That comparison is also what
substantiates the conftest's new claim that the harness, not the app, is what is
POSIX-bound.

Also green locally: isort, flake8, the black gate (run with the pinned
black==26.3.1, since a locally-installed 25.x reports a different verdict),
mypy --platform linux over all 1,038 source files (3 pre-existing
wecom/client.py errors in an untouched file come from a local aiohttp version
differing from CI's pin), the brand-name gate, and the subprocess-spawn audit.
The full backend suite was not completed on this host; CI is the backstop for it.

Related Issues

Fixes #4630

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bef513c

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root cause fixed at the right seam — prompts name a real interpreter, the venv-preference alternative is correctly rejected on injection grounds, and the follow-on Windows perms gap is closed rather than shipped.

[DESIGN-REVIEWED] bef513c

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of bef513ca1a0d33e819c4e7d67991c2fcd9116d80 — 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.

Reading the contract, the intent file, and the patch, then verifying claims against the repository (consumer counts, existing mechanisms, sibling sites) before emitting the structured review.

I've checked atomic_write.py, apps/interpreter.py, and grepped the sage app for remaining os.replace and mkstemp sites. Findings verified; emitting the review.

First-Principles-Verdict: CONCERNS

The description says the interpreter comes from resolve_app_python; the diff deliberately ships the opposite — and two undeclared behavior changes ride along.

What this change ships

Intent: make Code Review Sage work on Windows by naming a real interpreter instead of python3. FIX.

1. Sage no longer refuses to run on Windows — justified (fixes Code Review Sage's review worker hardcodes python3, which is not an interpreter on Windows #4630)
2. Review prompts carry an absolute interpreter path, worker quotes it itself — justified
3. Interpreter is the gateway's sys.executable, never the app venv — justified, but contradicts the description
4. Shipped skills switch commands to a <python> placeholder with a substitution note — justified
5. Staged records/reports locked owner-only via runtime helper, before payload — justified (named boundary)
6. runs.json/config writes gain random temp names + Windows rename retry — undeclared, rides along, derived harm
7. Run-registry save moved off the event loop (async _save_runs, six call sites) — undeclared, rides along, derived
8. Test suite's Windows gate re-justified (harness, not app) — justified
9. Prompt guard made interpreter-agnostic; four new test classes — justified
10. Spawn-audit allowlist entries for the new asyncio.run tests — mechanical

Watch

• Framing gap: the description says the prompts "name the interpreter resolved through the shared app policy (kiro_crew.apps.interpreter.resolve_app_python)"; the shipped docstring says "Deliberately NOT the shared resolve_app_python(app_root) policy" and a test forbids consulting app_root. The code's choice is the right one (worker-writable venv = injection vector); the description describes a different change. Same gap: "All nine call sites now route through one seam in sage_lib/store.py" — the two backend/routes.py sites route through kiro_crew.atomic_write, not that seam.
• Point patch on the rename retry: _write_runs's own comment cites the deploy pending confirmations can fail on a transient Windows replace sharing violation #4701/md-notebook: a contended rename loses the note save on Windows #4898 silently-lost-write class, yet 8 sibling os.replace( sites in the same app keep the non-retrying replace on the same now-Windows-live paths (grepped os.replace( under sage_lib/: discovery.py:382, learning.py:312, followup.py:283, report.py:814, results.py:81/255/471/531) — result records, whose loss is exactly the silent no-verdict this PR removes.
• followup.py:277 is the one staging site still on raw tempfile.mkstemp, so the new "everything staged is locked before payload" invariant in open_locked_temp's docstring is false for the session descriptor (1 sibling, grepped tempfile.mkstemp under sage_lib/).

Subtractions

• Replace the 8 hand-rolled mkstemp/write/replace/cleanup bodies behind open_locked_temp with atomic_write(..., restrict_to_owner=True) (guarded import, same shape as config_dir), deleting open_locked_temp — atomic_write's own module docstring claims all atomic-write sites, and this also closes the 8-site retry gap above.

[FIRST-PRINCIPLES-REVIEWED] bef513c

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The candidate list surfaced only one weak candidate. Let me verify it against the bar.

CANDIDATE 1 claims python_command() returning sys.executable is unguarded against an empty value. To ground (a) — a concrete input that occurs in practice — I'd need a supported deployment where sys.executable is empty. Kiro Crew ships via setuptools/PyPI (never frozen), and the gateway process runs under a real interpreter, so sys.executable is always a populated absolute path. The discovery pass itself scored this "low" and could not construct such a deployment. (a) fails; drop.

I re-derived the load-bearing invariants of this diff independently:

• All eight _save_runs() call sites (routes.py:261, 518, 938, 969, 1050, 1060, 1196, 1314) are now awaited; the only bare reference is the async def itself. No orphaned coroutine.
• Serialization (json.dumps(_RUNS)) happens on the loop before asyncio.to_thread, under the caller's lock — no torn write.
• open_locked_temp cleans up its own fd+temp on lockdown failure and returns neither handle, so caller finally/except OSError paths (results.py adopt_from_shared/publish_to_shared, discovery _write_repos, learning/report _atomic_write) never double-free or hit an unbound tmp.
• The lockdown now precedes the payload write (previously chmod followed it) — strictly safer, no regression.

Nothing survives at 80+.

No findings.

[OPUS-REVIEWED] bef513c

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

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

[chenmingwei23]

Dispositions for the GPT 5.6 findings on 24660cd7d747. Now at dab816112.

• owner lockdown blocks the gateway event loop on Windows (backend/routes.py:155) — fixed.

Windows review actions -> async handler -> _save_runs() -> synchronous icacls subprocess -> gateway and heartbeat freeze for up to 10 seconds.

Legitimate and mine: this PR turned a non-blocking os.chmod into store.restrict_to_owner, which spawns icacls with a 10s timeout on Windows, and all eight _save_runs() call sites sit in async def on the single loop. The neighbouring store.remove_run_dir was already offloaded, so sync IO there was known to be loop-hostile.

Not reverted, because reverting restores the silent-no-op lockdown this PR exists to close. Instead the function is split: _write_runs(payload) holds the blocking IO, and async def _save_runs() serializes _RUNS on the loop — deliberately, so the snapshot is taken under whatever lock the caller holds — then offloads only the write via asyncio.to_thread. Serializing inside the thread would have introduced the opposite defect (mutation mid-iteration). All eight sites now await _save_runs().

Pinned by test_the_lockdown_never_runs_on_the_event_loop, which records the thread _write_runs runs on and compares it to the loop's afterwards. It asserts outside the patch on purpose: _save_runs swallows every exception, so an AssertionError raised inside would have been logged and the test would have passed on a real regression. Mutation-checked — reverting the to_thread call makes it fail.

Completeness, since this narrowed one branch of a chain: every other site the PR routed through the same lockdown (results.py, report.py, learning.py, discovery.py, _write_review_section) reaches it only via asyncio.to_thread — _record_reviewed (500), add_repo/remove_repo (1476/1478), post_recorded (1003), _write_review_section (1662, and 1770 inside _delete_then_prune). routes.py:1586 needed no change for that reason.

• POSIX backslashes bypass required shell quoting (sage_lib/review_driver.py:280) — fixed.

POSIX interpreter under /srv/kiro\home/... -> python_command() -> _shell_ready() emits it bare -> shell consumes \ as an escape and invokes a nonexistent path -> review produces no result record.

Legitimate: a backslash is a legal POSIX filename character and an escape in an unquoted shell word, and the allowlist admitted it bare.

This is the second blocking finding in the review_driver.py:_shell_ready span (the first, on the previous round, was $ expanding inside a double-quoted PowerShell string). Rather than patch the character class a third time, the invariant: the POSIX branch no longer decides safety at all — it defers entirely to shlex.quote, which already emits an ordinary path bare and quotes anything unsafe. The allowlist survives only for Windows, where shlex encodes the wrong rules (backslash is the separator, not an escape) and the quoted form is a single-quoted PowerShell literal that interpolates nothing.

Both branches, stated explicitly since a fallback chain was narrowed: windows=False -> shlex.quote, correct for \, $, spaces, quotes and everything else POSIX shells treat specially. windows=True -> bare when the path matches the allowlist, else & '...' with '' escaping. test_a_posix_path_is_never_emitted_bare_when_the_shell_would_rewrite_it pins the class rather than the two characters.

[chenmingwei23]

Dispositions for Design Review's CONCERNS on 24660cd7d747. Now at 7e21c1a91.

• _shell_ready emits & '…', valid only in PowerShell and a syntax error in cmd.exe — and on Windows the quoted branch is the common case, not the edge — fixed.

On Windows the quoted branch is the common case, not the edge (C:\Users\First Last\.kiro\crew\… — usernames with spaces are the default layout), so if the worker session's shell is cmd, the exact silent no-result failure this PR removes comes back for a large slice of Windows users. Nothing in the diff pins which shell the ACP worker actually gets on Windows.

Correct on both counts, and I could not pin the shell: the worker's shell tool selects PowerShell or cmd by detection, so the diff was betting on one of two and a space-containing profile is ordinary rather than exceptional.

Taken with the two GPT findings in this same span across earlier rounds — $ expanding inside a double-quoted PowerShell string, then \ consumed as an escape by a POSIX shell — that is three defects from one root cause: rendering a shell-correct string for a shell this code cannot identify. So rather than fix the third instance, _shell_ready and its allowlist regex are deleted. python_command() now returns the resolved absolute path raw, and the prompt instructs the worker to quote it as its own shell requires. The worker knows its shell; this code does not. That removes the whole class rather than the case Design Review named, and it is a net deletion.

Adopted your suggestion directly ("drop dual-shell rendering and have the prompt state the raw absolute path"). TestInterpreterIsHandedOverRaw pins both halves: the resolved path is handed over unquoted and unescaped for space, $ and backslash paths alike, and both prompts carry the quoting instruction.

• The suite stays collection-gated on Windows, so the platform this PR exists for has zero automated end-to-end coverage — accepted-and-deferred.

Regression protection is the cross-platform pure-function tests plus one manual run. Acknowledged in the description, but it means the shell-assumption above can regress invisibly.

Legitimate and unchanged by this round. Making the suite Windows-native is real work with a decided shape, and it is not this PR: the harness asserts 0600 as st_mode bits (Windows expresses owner-only as a DACL and always reports 0o666), uses forward-slash path suffixes, and installs shell-script gh stubs the Windows runner cannot execute. Bundling that here would mean rewriting the harness in the same change that fixes the app, and the app fix is what closes #4630.

What this round does buy against invisible regression: the shell assumption you flagged no longer exists to regress, and the two guards added for it are pure functions that run on every platform CI covers. Filing the harness work as a follow-up issue naming those three concrete blockers.

• "pin the shell (or verify on a space-containing profile) before treating Code Review Sage's review worker hardcodes python3, which is not an interpreter on Windows #4630 closed" — fixed by removal rather than by pinning: with no shell-specific rendering left, a space-containing profile no longer takes a different code path from any other. The remaining requirement on the worker is one instruction in the prompt, which is where shell knowledge actually lives.

[chenmingwei23]

Dispositions for First Principles' CONCERNS on 24660cd7d747. Now at f9cd88279.

• skills/sage-review/SKILL.md:526 still reads python3 sage_lib/pipeline.py prepare, and the new skill test cannot see it — fixed.

the one surviving spelling of the root cause, and the new skill test cannot see it (^python3?\s is line-anchored; this line starts with >). Grep python3 under the app's skills/: 3 hits, 2 are the warning prose, this is the third.

Both halves correct, and the second half is the more useful one: the guard I added to prevent exactly this could never have seen it. A line-anchored pattern misses any command inside a blockquote, a list item, or indented prose — so the leftover could have come back at any time with the test still green.

The line now reads <python>, and the guard is no longer line-anchored: it matches an interpreter name followed by a *.py path anywhere in the line, with a lookbehind so the <python> placeholder itself does not trip it. Mutation-checked by restoring the python3 spelling on that exact blockquote line — the guard fails, and passes again once reverted. That is what makes the fix durable rather than a one-time correction.

• dashboard/handlers/core.py:774 emits & "{sys.executable}" double-quoted — the same rewrite risk — accepted-and-deferred, filed as Owner-only lockdown and Windows shell quoting have unfixed siblings outside Code Review Sage #4987.

which PowerShell would expand $/backtick in — the exact rewrite this PR's docstring warns about

Legitimate and verified. It is out of this PR's scope: a different subsystem, reached by a different surface, and pulling it in would widen a Sage fix into the dashboard handlers. Worth noting the disposition is not merely "later" — the sibling is arguably worse now, because after this round Sage does no shell-specific rendering at all, so that call site is the remaining precedent rather than one of two. The issue says so, and carries the reasoning that made this PR abandon per-shell rendering.

• The raw-os.chmod(0o600) cause has ~9 siblings outside this app — accepted-and-deferred, filed as Owner-only lockdown and Windows shell quoting have unfixed siblings outside Code Review Sage #4987 alongside the above.

The general sweep is genuinely larger than this change — accepted and deferred, but it is now the pattern's live precedent.

Agreed on all counts, including that framing. All nine sites are listed in the issue with the caveat that each needs its own reachability check rather than a blanket substitution. The issue also carries the trap this PR paid a review round to learn: restrict_to_owner spawns icacls on Windows, so any site that runs on the event loop must offload the write or it trades a silent permissions no-op for a gateway freeze — which is what happened here at routes.py:155 and is why the fix splits the snapshot from the write.

Design Review's separate harness concern is filed as #4988 with the measured Windows baseline, so the deferred work from both lanes is now written down rather than implied.

[chenmingwei23]

Disposition for Opus 4.8's BLOCK-MERGE on 24660cd7d747. Now at f9cd88279.

• routes.py:155 routes a subprocess.run(icacls) spawn onto the gateway event loop on the Windows path it simultaneously enables — fixed.

_record:239 / _handle_run_cancel:916 / _handle_run_delete:947 call _save_runs() on the loop under _LOCK → store.restrict_to_owner → platform_compat.restrict_to_owner spawns subprocess.run([icacls…], timeout=10) on the single event loop, freezing every task (chat turn, liveness heartbeat) up to 10s

Legitimate, and the trace is exactly right including the two details that made it actionable: that no-blocking-call-on-event-loop lists subprocess.run explicitly and matches routes.py, and that line 1586 is safe because _write_review_section is only ever reached via asyncio.to_thread.

Fixed by splitting rather than by the suggested per-call-site await asyncio.to_thread(_save_runs). Same effect at the eight sites, but the invariant lives in one place instead of eight, and it preserves something the per-site form would have broken: _RUNS is serialized on the loop, so the snapshot is still taken under whatever lock the caller holds. Offloading the whole function would have moved json.dumps into the worker, where a concurrent mutation could tear it — trading a freeze for a corrupt registry. _write_runs(payload) holds the blocking IO; async def _save_runs() snapshots then offloads.

Pinned by test_the_lockdown_never_runs_on_the_event_loop, which records the thread _write_runs executes on and compares it to the loop's after the call returns. The comparison is outside the patch deliberately: _save_runs swallows every exception, so an assertion raised inside the patched half would have been logged and the test would have passed on a real regression. Mutation-checked — reverting the to_thread call makes it fail.

Your local-mirror counterpart on the follow-up head independently traced every other site this PR routed through the same lockdown — results.py, report.py, learning.py, discovery.py, and both _write_review_section call sites — and confirmed each is already to_thread-wrapped, which is the completeness check this kind of one-branch narrowing needs. No further offload was required.

[chenmingwei23]

Disposition for the GPT 5.6 finding on f9cd88279. Now at 86fbf4ed6.

• Windows lockdown occurs only after sensitive bytes are written (sage_lib/results.py:257) — fixed, though not by the remedy you proposed.

Permissive inherited DACL -> newly enabled Windows review writes PR data before icacls -> another local account can read it.

The mechanism holds, and I verified the premise rather than taking it: mkstemp gives a POSIX mode of 0600 at creation, but on Windows access comes from the DACL and a new file simply inherits the directory's. Nothing tightens this app's data tree — store.ensure_layout uses a plain mkdir(parents=True, exist_ok=True), and the repo's own platform_compat.make_owner_only_dir is applied only to specific secret-bearing dirs elsewhere. So the window is real rather than theoretical, and it is a completeness gap in my own change: I made the lockdown effective on Windows and left it happening too late.

Fix: Restore the Windows refusal is rejected as disproportionate. That reverts the entire PR and the issue it closes (#4630) in order to correct an ordering bug, when the ordering is what is wrong. Reverting would also restore a worse state, since the same write-then-restrict order is what shipped on POSIX all along.

What landed instead: a single seam, store.open_locked_temp(directory, *, prefix, suffix), which creates the temp file and restricts it before returning the fd, so no caller can write a payload byte into an unrestricted file. The returned descriptor is opened before the DACL changes and Windows checks access at open time, so the write still succeeds. All seven mkstemp sites now obtain their temp file through it (results.py ×4, discovery.py, learning.py, report.py) and their post-write restrict_to_owner(tmp) calls are gone. The two routes.py writers use a deterministic .tmp name rather than mkstemp, so they create the file empty, restrict it, then write.

Two guards, both mutation-checked by removing the lockdown from the seam and confirming each fails: test_the_temp_file_is_locked_before_any_payload_byte_is_written asserts the order (lock before write) rather than the resulting mode, because mode bits cannot express this on Windows and ordering is the actual property; and test_a_failed_lockdown_does_not_leak_the_descriptor covers the new failure path, where the caller never receives the fd and so nothing else could close it.

Left deliberately out of scope: the containing directories are still not owner-only, which is the deeper fix (and would also stop the file names disclosing which PRs were reviewed). That belongs with the other lockdown siblings in #4987, not in a change whose subject is the interpreter.

Separately, the four red backend shards were not a product defect. test/test_spawn_audit.py's AST heuristic matches any attribute call named run, so the asyncio.run(...) calls my tests use to drive the now-coroutine _save_runs were counted as unrouted subprocess spawns. The file already carries a dozen entries for exactly that false positive, and the real icacls spawn (platform_compat.py::restrict_to_owner) is allowlisted there already, so the four test keys are now listed with that justification. Coverage Gate was a pure cascade of backend-test=failure, not a coverage shortfall.

[chenmingwei23]

Disposition for the Opus 4.8 observation on 86fbf4ed6 (lane PASSED — the candidate was falsified down to non-blocking). Now at bbf4f045d.

• open_locked_temp orphans the mkstemp file when the lockdown raises (sage_lib/store.py) — fixed, despite the lane correctly declining to block on it.

when restrict_to_owner raises inside open_locked_temp, os.close(fd) runs but the mkstemp file is never unlinked, and since the exception propagates out of the fd, tmp = ... assignment before the caller enters its try/finally, no caller cleans it up

The mechanism is exactly right, including the part that makes it real: the exception escapes during the tuple assignment, so the caller's try/finally — which is what removes the temp on every other path — has not been entered yet. Nothing downstream could clean it up.

Your falsification is also right, and is why this was advisory rather than blocking: on POSIX (the only platform the suite runs, since the conftest still skips Windows) restrict_to_owner is a chmod on a freshly-created file this process owns and does not raise; and because the lockdown deliberately precedes every write, the orphaned artifact is always zero bytes, so no change content is exposed. There is no security dimension. I am fixing it anyway because the leak is in code this PR introduces, it is two lines, and a stray empty temp file per failed write in the app's data dir is still litter that nothing else reaps.

The failure path now closes the descriptor and unlinks the temp, best-effort (OSError suppressed — the point is not to mask the original exception, which still propagates). The existing guard was widened from descriptor-only to both, and renamed accordingly: it snapshots the directory listing before the call and asserts it is unchanged after. Mutation-checked by deleting the unlink — the test fails, and passes again once restored.

Worth recording that this is the second finding this lane produced whose value was in the falsification, not the verdict: naming why it does not block (empty artifact, untested platform) is what let me fix it without inflating it into a blocker or widening the diff.

[chenmingwei23]

Disposition for the GPT 5.6 BLOCKING finding on bbf4f045d. Fixed in 65dc60ae8.

• Fixed temp names follow worker-planted symlinks (backend/routes.py:161, residual/security) — fixed.

Prompt-injected worker plants either .tmp symlink -> gateway save follows it outside the sandbox -> arbitrary user-owned file is permission-changed and overwritten.
Fix: use store.open_locked_temp, write through its descriptor, then replace.

Legitimate, and I verified the mechanism rather than taking it on the wording. Both sites built a fully predictable path (f.with_name(f.name + ".tmp")), and Path.touch() opens through a symlink — so all three steps landed on the planted target: touch created/opened it, restrict_to_owner changed its ACL, and write_text overwrote it. What settled it is that this threat model is already asserted in this tree: tests/test_record_adoption.py::test_a_planted_symlink_is_removed_not_followed exists precisely because the review worker writes into this directory and is prompt-injectable.

Both writes now go through the store.open_locked_temp seam introduced earlier in this PR: mkstemp picks an unpredictable name and creates with O_EXCL, which cannot open a path that already exists — symlink included — and the lockdown is applied before the descriptor is handed back. The payload goes through that fd, then os.replace, which replaces the name rather than following a link. The failure path closes the fd and unlinks the temp.

Worth noting this fix was available two rounds ago and I did not take it: when the lockdown-ordering finding landed, I routed the seven mkstemp sites through open_locked_temp but left these two as touch() → restrict_to_owner → write_text because they used Path.write_text and that was the smaller edit. That inconsistency is exactly what this finding found. Span tally for the record: routes.py temp-write path — 3 rounds (_save_runs on the event loop, lockdown ordering, now symlink-following). All three trace to one root cause — writing sensitive bytes through a path the worker can predict or race — and the seam is now the single way this module creates a temp file, so there is no remaining variant to hand out.

Two guards, because the natural one cannot run where it matters:

• test_a_planted_tmp_symlink_is_not_followed — plants a symlink at the old predictable path and asserts its target is untouched. Skips on Windows (symlink creation needs a privilege CI does not grant), like 15 existing tests in that file.
• test_the_predictable_tmp_name_is_never_used — symlink-free twin asserting anything pre-placed at runs.json.tmp survives the write. This one does run on Windows, which is the platform this PR is about. Mutation-checked: restoring the old touch()+write_text fails it.

One thing I got wrong mid-round and am flagging rather than quietly dropping. Checking whether the fix reintroduced the no-blocking-call-on-event-loop defect this PR already fixed once (open_locked_temp spawns icacls on Windows), I wrote an audit whose enclosing-function scan was anchored at column 0. It missed an indented nested def and reported _write_review_section as on-loop at the namespace-prune site. It is not: that call sits inside _delete_then_prune, a sync closure already dispatched wholesale via await asyncio.to_thread(...). I "fixed" a non-bug, the syntax error caught it immediately, and it is reverted — the diff contains no such change. The corrected audit, independently reproduced by the local Opus mirror across the wider sage_lib surface, is that every icacls-reaching path is offloaded: _write_runs via _save_runs, both _write_review_section sites, discovery.add_repo/remove_repo, learning.create_namespace/delete_namespace, and every results.* writer inside the offloaded run_review / post_recorded / _record_reviewed.

[chenmingwei23]

Disposition for the GPT 5.6 BLOCKING finding on 65dc60ae8. Fixed in 2ff1c1676.

• The worker can persist an executable as the next review's interpreter (sage_lib/review_driver.py, residual/security) — fixed.

Prompt-injected review -> plants an executable .venv in its writable app root -> subsequent reviews execute it and accept forged records.
Fix: Resolve without the worker-writable app root, e.g. resolve_app_python(None).

Legitimate, and the premise is the part I verified rather than assumed: store.app_root() returns crew_home() / "apps" / APP_NAME, so it resolves under KIROCREW_HOME — the writable data home, not the read-only installed package tree. The review worker writes into that tree and is prompt-injectable, so a planted .venv/Scripts/python.exe would be executed by the next review. That is persistence into a later run, which is a genuine escalation under the threat model this app already assumes elsewhere (the planted-symlink guards, the owner-only lockdowns, and the open_locked_temp seam added earlier in this PR).

python_command() now returns sys.executable directly. I did not write resolve_app_python(None) as suggested, because with root=None that function's only reachable branch is its sys.executable fallback — the call would be indirection through a resolver that can no longer resolve anything, which reads as though an app venv might still be selected. The explicit form says what actually happens, and the docstring carries the reason so the next reader does not "restore" the policy call.

Nothing is given up by declining the venv preference here, and I checked rather than assuming. That preference exists so an app's own dependencies are importable. This app ships no requirements.txt, and an AST scan of every module in sage_lib/ finds exactly one non-stdlib, non-local import across the package: kiro_crew itself. Under sys.executable that is importable by construction, because sys.executable is the interpreter running the gateway. An app venv would in fact have been the weaker choice — it need not have kiro_crew installed at all, which would fail every sage_lib import rather than just one review.

Two guards, and the second is the one that matters for this span:

• test_the_interpreter_is_never_taken_from_a_worker_writable_path patches store.app_root to raise, so any reintroduction of that lookup fails the suite rather than silently returning a worker-influenced path. Mutation-checked by restoring the old call: it fails, and passes again once reverted.
• test_the_resolved_path_is_not_quoted_or_escaped was re-pointed at the new seam (sys.executable) instead of being deleted. It injects a path with a space, one with a $, and one with a backslash run — the three defects rounds 1–3 produced — and asserts each arrives verbatim. Losing it while changing where the path comes from would have dropped the guard on this span's whole earlier history.

Span tally, recorded so a later round can see it: review_driver.py interpreter handling — 4 rounds. Rounds 1–3 were all quoting the path ($ expansion in a double-quoted PowerShell string; \ consumed as an escape by a POSIX shell; & '...' being a cmd.exe syntax error while spaces in C:\Users\First Last\… make the quoted branch the common Windows case) and were closed structurally by deleting the helper that rendered a shell-specific string — the worker quotes for its own shell now. This round is a different axis: where the path comes from. Both axes are now pinned by a test, and the function is three lines with no branches, so there is no remaining variant of either question to hand out.

This one needed a maintainer ruling rather than another patch, and got one: the choice between sys.executable, resolving an app venv once at gateway start, and accepting the risk is a trust-boundary decision, not a code detail. The verification above is what made it cheap to decide — with no app dependencies to lose, the narrowest option costs nothing.

[chenmingwei23]

Dispositions for Design Review's CONCERNS on 2ff1c16766c9. Now at bef513ca1.

• Description ↔ diff mismatch on the core decision — fixed.

The description says the prompts use the shared resolve_app_python policy ("the app's own venv when it has one") and that spaced paths are "quoted literally" by the driver, and cites a TestShellReadyInterpreter. The shipped code does the opposite on both counts […] Update the description before merge.

Correct on all three counts, and this is the concern I most needed raised: a green rollup would have merged a record documenting the design the diff rejects. Every claim you named was true of an earlier revision and false of the shipped one. The body now states that sys.executable is used and that resolve_app_python(app_root) is deliberately declined, with the worker-writable-app_root reasoning inline; the "quoted literally" paragraph is replaced by the raw-handover rationale; and the TestShellReadyInterpreter reference is replaced with TestInterpreterIsHandedOverRaw, which is the test that actually exists. I also corrected two stale numbers the mismatch had hidden — the local baseline (750, not 747) and the mypy file count.

• The silent no-result failure survives one path — rebutted, on evidence, and it is pre-existing code rather than anything this diff adds.

Confirm the driver surfaces "review produced no result record" as an explicit error rather than an empty verdict, so the residual case fails loud.

It already does. sage_lib/review_driver.py has, immediately after the turn-level failure branch:

if not rec["deep_reviewed"]:
    rec["skipped_reason"] = "no_review_recorded"  # turn completed but wrote no review
    progress(change_id, "failed", {"error": "review produced no result record"})
    return rec

So a worker that ignores the quoting instruction lands in a failed progress event carrying that exact string, not an empty verdict. Worth noting the guard is deliberately ordered after the "never discard a record that DID land" check, so a trailing abnormal stop cannot drop verdicts that were already written. Nothing to change; the residual case fails loud today.

• Undescribed hardening riding along — fixed (declared, not removed).

the predictable runs.json.tmp → O_EXCL open_locked_temp symlink-plant fix and the async _save_runs offload are real behavior changes absent from the description […] name them in the PR text so they're findable later.

Both are now their own named bullets under "What changed", each with the mechanism that forced it: the predictable temp name was plantable in a worker-writable directory and Path.touch() follows a symlink, and restrict_to_owner spawns icacls on Windows so the registry write can no longer sit on the gateway loop. In this round those two writers went one step further and now call kiro_crew.atomic_write.atomic_write(..., restrict_to_owner=True) — see the First Principles disposition for why — which is also declared there.

[chenmingwei23]

Dispositions for First Principles' CONCERNS on 2ff1c16766c9. Now at bef513ca1.

• Item 4: description names resolve_app_python, diff ships sys.executable — fixed.

The shipped choice is the safer one; the description documents the rejected one. Fix the description.

Agreed, and the framing is the useful part: the diff was right and the text was wrong, so the fix belongs in the text. The body now says sys.executable is used and that the shared policy is deliberately declined, with the reason (an app_root under KIROCREW_HOME is worker-writable and the worker is prompt-injectable, so a planted .venv/Scripts/python.exe executes on the next review). It also records why declining costs nothing: no requirements.txt, and an AST scan of every module in sage_lib/ finds kiro_crew as the only non-stdlib import — importable under sys.executable by construction, whereas an app venv need not carry it at all.

• Items 6 and 7 ride along undeclared — fixed (declared, not deleted).

Both have named causes (worker-writable tree; icacls blocking the loop), so declare them rather than delete them.

Right on both the causes and the remedy. Each is now its own bullet naming the mechanism that forced it. They are not separable from the fix: item 5 is what makes restrict_to_owner run at all, item 7 exists only because that helper spawns icacls, and item 6 exists only because the same tree the lockdown protects is one the worker can pre-plant into.

• Root-cause siblings: ~15 chmod(..., 0o600) sites outside this app — accepted-and-deferred, and already filed.

The general mechanism already exists (platform_compat.restrict_to_owner); accepted-and-deferred, not a demand.

Filed as #4987 before this round, with the sites enumerated and the trap that makes the sweep non-trivial recorded in the body: restrict_to_owner spawns icacls, so a site sitting on the gateway event loop cannot simply be swapped — which is exactly the defect an earlier round of this PR had to fix. Deferring is right for scope, and the issue names work someone can pick up rather than a question.

• Subtractions: replace the hand-rolled writer bodies with atomic_write — fixed. This was the most valuable item in either review.

Replace the hand-rolled bodies of _write_runs and _write_review_section […] with kiro_crew.atomic_write.atomic_write(path, payload, restrict_to_owner=True) […] it already does mkstemp, lockdown-before-payload, replace-with-Windows-retry, and temp cleanup

Verified against the helper rather than taken on the claim, because adopting it would have regressed two properties earlier rounds fixed if the ordering differed. It does not: with restrict_to_owner=True it uses tempfile.mkstemp (unpredictable name), applies platform_compat.restrict_to_owner to the temp before the payload is written, and calls _refuse_linked_parent(path) before mkdir — a planted-parent-link refusal the hand-rolled version did not have at all. So the substitution is strictly stronger, not merely shorter.

It also closes a latent defect I had introduced and not noticed: my writers replaced through a bare os.replace, while atomic_write uses replace_with_retry. That is the transient-Windows-sharing-violation class this repository already tracks in #4701 and #4898 — so the subtraction removes code and fixes a bug, which is why I took it now rather than deferring it.

The constraint you identified is respected: open_locked_temp stays for the seven sage_lib sites, whose standalone-import fallback atomic_write cannot serve; only the two routes.py sites moved, and routes.py imports kiro_crew unconditionally at line 44. Dropping the now-unused import os was verified by whole-file scan (zero remaining os. references) rather than inferred from flake8 alone.

Recording one span fact for the next round: routes.py's temp-write path had taken three rounds of findings (event-loop blocking, lockdown ordering, symlink-following). Adopting the shared helper is the invariant that makes all three unreachable at once, which is what the same-span rule asks for instead of a fourth point patch.

[chenmingwei23]

First Principles (Fable 5) — CONCERNS on bef513ca1a0d, answered per item

• Framing gap: the description names resolve_app_python and claims "all nine call sites" route through one seam — rebutted (stale read of the body).

  the description says the prompts "name the interpreter resolved through the shared app policy (kiro_crew.apps.interpreter.resolve_app_python)"; the shipped docstring says "Deliberately NOT the shared resolve_app_python(app_root) policy" ... Same gap: "All nine call sites now route through one seam in sage_lib/store.py" — the two backend/routes.py sites route through kiro_crew.atomic_write, not that seam.

  Both quoted strings are from the PREVIOUS body, which was rewritten for exactly this reason in the round that produced this head. Read back from the live PR body: resolve_app_python appears once, as "This is deliberately not the shared resolve_app_python(app_root) policy"; the phrase "nine call sites" does not appear at all (the body says "The sage_lib call sites now route through one seam"); and the routes.py pair is described as "Both now call kiro_crew.atomic_write.atomic_write(...". The code and the description already agree — no change needed, and none made.

• 8 sibling os.replace( sites in sage_lib/ keep the non-retrying replace — needs-a-decision (verified, but outside this diff).

  8 sibling os.replace( sites in the same app keep the non-retrying replace on the same now-Windows-live paths (grepped os.replace( under sage_lib/: discovery.py:382, learning.py:312, followup.py:283, report.py:814, results.py:81/255/471/531)

  Verified — a whole-file scan of sage_lib/*.py returns exactly those eight lines, and the reasoning holds: this PR is what makes those paths reachable on Windows, so the deploy pending confirmations can fail on a transient Windows replace sharing violation #4701/md-notebook: a contended rename loses the note save on Windows #4898 sharing-violation loss class becomes newly live there. What makes it a maintainer call rather than a fix is that none of the eight lines is in this PR's diff, and the PR is at 63/63 green after seven review rounds. Sweeping them re-arms every lane on ~8 files of new surface. Raymond's ruling requested: land as-is with a follow-up issue, or widen now.

• followup.py:277 still stages on raw tempfile.mkstemp, so open_locked_temp's docstring invariant is overbroad — needs-a-decision (verified, same ruling).

  followup.py:277 is the one staging site still on raw tempfile.mkstemp, so the new "everything staged is locked before payload" invariant in open_locked_temp's docstring is false for the session descriptor

  Verified — tempfile.mkstemp under sage_lib/ resolves to followup.py:277 plus store.py:175 (the seam itself). The docstring overclaims by one site. The minimal honest correction is either routing that site through the seam or narrowing the docstring; both fall inside the same scope question as the item above, so they are answered together rather than half-applied.

• Subtractions: delete open_locked_temp, route all sites through atomic_write(..., restrict_to_owner=True) — needs-a-decision (this is the structural fix, and it is the 5th round in this span).

  Replace the 8 hand-rolled mkstemp/write/replace/cleanup bodies behind open_locked_temp with atomic_write(..., restrict_to_owner=True) ... deleting open_locked_temp

  Recorded for the next round: the staging/temp-write span has now taken findings in five consecutive rounds — open_locked_temp lockdown ordering, the fd/temp leak on lockdown failure, the predictable .tmp symlink plant in routes.py, routes.py adopting atomic_write, and now the eight siblings plus the seam's deletion. Per the same-span rule this stops being a patch queue and becomes one restructure decision, which is why it is put to the maintainer instead of applied as an eighth push. The proposal is sound on its merits — atomic_write already carries replace_with_retry, _refuse_linked_parent, and owner-restriction before first write, which is a strict superset of what the seam guarantees.