fix(themes): enable theme pack routes on Windows

[iamwhatever]

Successor to #3293 by @leonlaiyc — this is their change, rebased. The single commit here retains Leon as git Author; I am only the committer. #3293 had gone 1234 commits behind main with conflicts in three files, so rather than force-push someone else's fork I replayed the commit onto current main and resolved the conflicts. Credit for the design and the reasoning below is theirs. The five hardening items in "What changed" beyond the gate deletion were added by me during review rounds on this PR.

Problem / Motivation

Theme-pack install, detail, assets, overlays, topbars and removal all returned HTTP 501 "theme packs are not yet supported on Windows". The gate existed because every one of those routes reads through the O_NOFOLLOW + fd-real-path containment chokepoint (safe_read_file_bytes_nolink), and hooks._fd_real_path had no Windows implementation — it returned None there, so the chokepoint fail-closed on every read. Rather than fail opaquely, the routes were gated honestly.

That premise no longer holds. _fd_real_path gained a complete Windows branch on main independently of this work (hooks.py:1826: ctypes.WinDLL("kernel32") + msvcrt.get_osfhandle → GetFinalPathNameByHandleW, with \\?\UNC\ / \\?\ prefix normalization and fail-closed return None). The gate has therefore been blocking a capability that works, and the comment above it asserted something about main that had become false.

Why it matters

Windows is a first-class platform, and theme packs are one of the few user-facing surfaces still refused outright there. Every Windows user hits a 501 on install and on every pack-backed asset route. The stale comment compounded it: anyone reading themes.py to decide whether the gate was still needed was told the Windows implementation did not exist.

What changed (motivation → approach → change)

Symptom: 501 on all pack routes on Windows. Root cause: a gate whose stated justification is no longer true. Change: remove _THEMES_WIN_UNSUPPORTED / _win_unsupported_response, and exercise the real containment path on Windows.

The gate was hiding a CLASS of Windows-specific hazards, not one bug. An earlier revision of this description said "one failure", which undersold the surface a reviewer has to check — corrected here. Six consequence items ship alongside the deletion, and all six are reachable only because the gate is gone:

1. GitHub-source install on a sandbox-less host answers 503, not 500. A github source clones through the sandbox chokepoint, which fail-closes wherever no OS sandbox backend exists — Windows, and every GitHub runner. _clone_github catches the typed SandboxUnavailableError and answers 503 with theme_install_sandbox_unavailable. 503 because this is a host capability gap: the request was well-formed and no retry or different body carries the remedy. No unsandboxed retry — the URL is user-influenced and git clone executes remote content, which is why the spawn is chokepointed at all. Local sources spawn nothing and stay installable, so the capability this change enables is not re-gated behind a new status.
2. UNC pack paths are refused before any filesystem access. A UNC path names a HOST, so stat-ing user-supplied \\attacker\share makes Windows open an SMB connection and authenticate, handing the gateway's credentials to a host the caller chose. _resolve_local_source now screens the raw and expanded text lexically via hooks.is_unc_shape + unc_probe_allowed — the same chokepoint acp/prompt_blocks.py:171 applies to attachment paths — before is_dir(). Not a blanket ban: unc_probe_allowed keeps the one legitimate case working, a roaming profile whose home is itself a UNC share.
3. A junction as the pack ROOT is refused. Path.is_symlink() returns False for a Windows junction, so a junction root passed the old check and was resolved through. Now uses platform_compat.is_link_or_junction.
4. A junction SUBDIRECTORY is refused during the copy walk. Same predicate gap: os.path.islink is False for a junction and os.walk reports one as an ordinary directory, so a pack carrying a junction back to its own root recursed until a path-length OSError escaped as a 500.
5. Asset/overlay/topbar path resolution moved off the event loop. _resolve_theme_asset does resolve()/is_file(), which are SMB-backed on a UNC data home; the three routes already offloaded their read to discovery_executor() but called the resolve inline. Known incomplete — see the note below.
6. A LINKED ANCESTOR of the pack path is refused, before anything probes it. Items 2 and 3 each close half of this and leave the seam between them: the UNC screen is purely lexical, so a local-looking path sails through it, and is_link_or_junction tests only the final component. A path sitting beneath an ancestor link whose TARGET is \\attacker\share was caught by neither, and resolving that chain is what opens the attacker's SMB connection. New platform_compat.first_linked_ancestor, called from _resolve_local_source. Two things about it are load-bearing and I got both wrong on the first attempt — see the review disposition comments:
   • It runs before the leaf check, not after. is_link_or_junction is an lstat, and while an lstat does not follow the final component it still resolves every ancestor — so a leaf-first order makes the leaf probe itself traverse the junction and open the connection the screen exists to prevent. Pinned by test_the_walk_runs_BEFORE_the_leaf_probe, which wires the leaf predicate to raise so an order regression explodes rather than passing.
   • It is IS_WINDOWS-gated. Ungated it refuses legitimate POSIX paths: on macOS /tmp and /var are symlinks to /private/*, so any install from a temp dir would 400 on a platform this PR does not otherwise touch. The gate is not merely damage control — on POSIX the probe is not the harm, and the real guard is is_sensitive_path applied to the resolved path further down, so the walk buys nothing there. Pinned by test_a_posix_path_beneath_a_linked_ancestor_is_still_accepted.
   • The walk is root-first and stops at the first hit, so each lstat runs only after every ancestor above it is known clean and the probe never traverses one. The refusal reuses the leaf case's exact message: which ancestor is linked is filesystem layout, and the caller supplied a path to guess at it.

The five screens in _resolve_local_source now run in strictly increasing order of filesystem exposure — lexical UNC (touches nothing) → ancestor walk (root-first lstats) → leaf link check (one lstat) → is_dir() (resolves the chain) → resolve() + is_sensitive_path. That ordering is the invariant to preserve when editing this function; the round-5 bug was a screen in the right place doing the right thing one line too late.

The symlink capability probe keeps the fail-soft contract it already has on main: any failure to create a symlink means "this host cannot", and the tests it guards skip. The probe runs at conftest import time, so a propagating error is not a loud signal on one test — it is a collection error that takes down the entire session, including every test that never touches a symlink.

Known incomplete, both now tracked

Both reviews asked that these be committed follow-ups rather than standing maybes. Filed:

• Move the remaining six api_theme_detail stats off the event loop #5963 — item 5 covers three of nine call sites. Six inline target.exists() / dir_target.is_dir() calls remain on the event loop in api_theme_detail (themes.py:635–715). They are SMB-backed on a UNC data home and the 501 gate covered them too, so they are newly reachable by this PR's own argument. Item 5 exists because it was a GPT blocking finding here; the siblings are the same fix and belong together, which is why they are one issue rather than a widening of this PR into a sixth round.
• Ancestor-link seam remains at the non-themes is_unc_shape call sites #5962 — item 6's root cause exists at the other is_unc_shape sites. A lexical screen followed by something that resolves the chain. Raised by First Principles; I checked all four production sites and the claim does not generalise evenly, so the issue records them individually: acp/prompt_blocks.py:171 (is_file()) and hooks.py:1673 (realpath — not named in the review) are clear matches; memory.py:592 already checks the workspace leaf and reasons about traversal, so it is half-defended; messaging/outbound_files.py:373 is followed by purely lexical checks and needs tracing before it is called a match. Three unrelated subsystems, so not in this PR.

Tests

• test/test_symlink_capability_probe.py (new) pins the fail-soft probe contract, including the Windows privilege shape — which carries an unrelated errno alongside winerror 1314 and so has to survive on the OSError type alone.
• test_a_junction_subdirectory_is_refused_like_a_symlink — proven red-before with the import intact and only the call reverted (DID NOT RAISE ValueError); stashing the whole file instead fails on a missing monkeypatch target and looks like proof without being it.
• test_a_local_path_beneath_a_linked_ancestor_is_refused — the item 6 vector. Proven red-before by disabling only the guard's call line while still evaluating first_linked_ancestor(p), so a dropped import cannot masquerade as the proof; with the guard off the function returns the path resolved through the link, which is the vector itself rather than a generic assertion miss.
• test_it_reports_the_OUTERMOST_link_when_several_are_nested — pins the root-first order directly, since that ordering is the safety property and a correct-looking return value can still have been reached by an unsafe probe.
• test_an_unlinked_local_path_is_still_accepted and test_the_error_does_not_disclose_which_ancestor_was_linked — the two ways this fix could go wrong: refusing ordinary nesting, and leaking filesystem layout in the refusal.
• test_a_unc_source_is_refused_without_touching_the_filesystem — wires both Path.is_dir and is_link_or_junction to raise, so the assertion is that no filesystem call happens, not merely that the path is refused.
• test_a_roaming_profile_unc_source_gets_past_the_shape_screen — the exact inverse, guarding against over-blocking the legitimate roaming-profile case.
• test_a_junction_root_is_refused — the root-junction branch.
• test_dashboard_themes_coverage.py / test_theme_install.py — 501 assertions replaced with coverage of the real Windows path and the 503 translation; Windows-skip scaffolding dropped.
• error-code-baseline.json regenerated: missing_code 1359 → 1358 (the removed 501 carried no machine-readable code). The regeneration also corrects _compliant 1167 → 1189; only the three totals lines changed and the per-file map is byte-identical, so that is pre-existing staleness, not an effect of this diff.

Manual verification

N/A from this host — every behaviour this change enables is win32-only and this is a Linux machine, which is also why #3293 could not self-verify. The diff therefore leans on the capability probe and typed refusals rather than platform-conditional skips. The 503 translation and the UNC/junction screens are reachable on Linux CI (no sandbox backend on runners; both predicates are lexical or stat-based), so those are covered by the suite rather than by inspection.

Screenshots / video

Why no screenshot: backend handler + tests + docs only. No dashboard component, layout, or string renders differently; the user-visible change is an HTTP status on Windows, which has no pixel form on this platform.

Declined here, with reason

The Design review suggests replacing the err == _THEME_GIT_SANDBOX_UNAVAILABLE string-identity check (compared in both _do_install and api_themes_install) with a typed status returned from _clone_github, so rewording the user-facing text cannot silently degrade 503+code back to a bare 400. That is a fair fragility and it is advisory, not blocking. Not taken here: the degradation it guards against is already pinned by the 503 translation tests, so the refactor buys robustness against a future edit rather than fixing present behaviour — and this PR is five review rounds deep on a security seam. Worth doing on its own.

Related Issues

Closes #311
Supersedes #3293
Follow-ups: #5962, #5963

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Removes a gate whose premise it verifies is dead on main, and hardens every hazard the removal newly reaches — typed, layered, and follow-ups filed.

[DESIGN-REVIEWED] f86d1df

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f86d1df

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of f86d1dfca5c86dcd8a928da9f4a0a7dfeb8a483d — 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 evidence checked. Writing the review now.

First-Principles-Verdict: CONCERNS

Every item is a derived consequence of one cause-level deletion, but the new ancestor-link screen guards 1 of 4 sites sharing its own stated root cause.

What this change ships

Intent: let Windows users install and use theme packs, whose routes 501'd on a premise now false — a FIX.

1. Theme-pack install/detail/assets/overlays/topbars/removal work on Windows — justified, cause-level (_fd_real_path's Windows branch verified at hooks.py:1991)
2. GitHub-source install on a sandbox-less host answers 503 + code instead of 500 — justified
3. UNC pack path refused before any filesystem access — justified (external-content boundary)
4. Junction as pack root refused — justified (platform predicate gap)
5. Junction subdirectory refused during the copy walk — justified (same gap)
6. Path beneath a linked ancestor refused; new first_linked_ancestor — justified, but point-applied (see Watch)
7. Asset/overlay/topbar path checks moved off the event loop — declared symptom-share: 3 of 9 sites, tracked (Move the remaining six api_theme_detail stats off the event loop #5963)
8. Docs rows + error-code baseline updated — mandated same-commit
9. New tests pin the conftest symlink probe's fail-soft contract — rides along, test-only, harmless

Watch

• The PR's own argument — "a lexical UNC screen cannot catch it, because only the link's target is UNC-shaped" — indicts every other lexical-screen-then-stat site. Grepped is_unc_shape.*unc_probe_allowed: 4 production siblings; memory.py:600 recorded the opposite decision with rationale, leaving 3 with no ancestor screen: hooks.py:1838 (validate_file_path, then realpath), acp/prompt_blocks.py:171, messaging/outbound_files.py:373. The description names two tracked follow-ups but is truncated in my copy after Move the remaining six api_theme_detail stats off the event loop #5963 — confirm the second one covers these, and reconcile per-site against memory.py's recorded "ancestors are not agent-writable" rationale rather than blanket-applying the walk.
• first_linked_ancestor ships in shared platform_compat with exactly 1 production consumer (themes.py:238, counted); acceptable only because the 3 siblings above are its intended future callers — if the follow-up decides against them, it belongs inline in themes.py.

[FIRST-PRINCIPLES-REVIEWED] f86d1df

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Looking at CANDIDATE 1, I need to verify three things independently: (a) a concrete input that occurs in practice, (b) the call path, (c) an observable wrong outcome.

The DELETE→shutil.rmtree reachability on Windows is real (the diff removed the 501 gate at themes.py:646). But the candidate collapses on both the input and the outcome:

(a) — the "planted junction" input is a "could", not a practiced condition. The only API path that creates an installed theme directory is _do_install→_copy_installed_theme, and this very diff hardens that walk to refuse junction subdirectories (is_link_or_junction, themes.py:330-336), while _resolve_local_source now refuses a junction root (themes.py:340-343). So a junction cannot enter the themes tree through the theme surface. The candidate's actual input is an actor planting a junction directly on the filesystem — but any actor who can create a junction under ~/.kiro/crew/themes/ and reach the token-authed DELETE endpoint already has the access to delete the target directly; no boundary is crossed that the endpoint uniquely bridges. That is analogy-as-requirement, not a derived trigger.

(c) — the wrong outcome requires assuming stdlib code I cannot open, and is version-dependent. Whether shutil.rmtree recurses into a junction on "pre-3.12 Windows" depends entirely on CPython's _rmtree_unsafe/os.lstat reparse-point handling, which is not in this repo and which the candidate itself hedges ("3.12 is safe", "could not confirm the Python version"). The falsification rule forbids establishing an outcome that "requires assuming code you did not open," and forbids survivors whose input reads as "if a caller were to."

The candidate's own confidence is "medium," and it dies on both the input (a "could") and the mechanism (unopened, version-dependent stdlib). Dropped.

Scanning the remainder of the diff (ancestor-link screen, sandbox-unavailable 503 mapping, executor offloading of the asset/overlay/topbar resolvers, first_linked_ancestor root-first walk) surfaces no reachable regression at the required bar — it is consistent hardening with matching tests.

No findings.

[OPUS-REVIEWED] f86d1df

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

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

[iamwhatever]

Disposition for the GPT BLOCKING finding on 8d841b7a6b3e:

• Windows installs can recurse through junction cycles (themes.py:527): fixed in 9db1a5319.

  Local pack with a junction back to its root -> install -> _copy_installed_theme follows it -> path-length OSError escapes as HTTP 500.

  The finding holds exactly as written, and the mechanism is worth stating precisely because it is easy to misread as already-guarded. _copy_installed_theme did carry a directory-link guard, but it tested os.path.islink, and os.path.islink returns False for a Windows junction — while os.walk reports a junction as an ordinary directory. So the existing guard was a no-op against junctions specifically, and lifting the 501 made that reachable.

  I did not take the suggested fix. The suggestion was:

  Fix: Restore the removed Windows install guard until the copy walk rejects junctions.

  Restoring the 501 would revert the entire purpose of this PR, and the same sentence names the better condition — "until the copy walk rejects junctions". So the walk now rejects them. The guard calls platform_compat.is_link_or_junction, which is the repo's already-audited predicate for this exact hazard; its own docstring records the failure mode ("os.path.islink returns False for a junction, so a caller that only checks islink would treat a junction as a real directory and rmtree THROUGH it"). Reusing it rather than adding a second reparse-point check keeps one definition of "link-like directory" in the codebase, and it already handles the Python 3.10/3.11 gap where os.path.isjunction does not exist.

  Regression test, proven red-before: TestCopyInstalledThemeSwapGuards::test_a_junction_subdirectory_is_refused_like_a_symlink. Worth noting how it was proven, because the obvious method gives a false positive: stashing the whole file also removes the is_link_or_junction import, so the test then fails with AttributeError: module has no attribute 'is_link_or_junction' — a missing monkeypatch target, not the guard's behaviour. Reverting only the call line and keeping the import yields the real signal, Failed: DID NOT RAISE <class 'ValueError'>. It is patched on the themes namespace rather than on platform_compat because the module binds the name with from ... import.

  Scope note: themes.py has exactly one os.walk site, so there is no sibling caller left on the old predicate — the only remaining os.path.islink text in the file is the mention inside the new explanatory comment.

Gates after the fix: 545 passed (up one, the new test), isort / flake8 / mypy clean.

[iamwhatever]

Dispositions for the First Principles BLOCK on 8d841b7a6b3e, one per finding:

• Blocker — the added testing-conventions.md paragraph documents a contract that does not exist: fixed in 9db1a5319 by deleting the paragraph, which is the subtraction the review asked for.

  It says the probe "skips only when Windows reports ERROR_PRIVILEGE_NOT_HELD (WinError 1314)… Other filesystem errors propagate so a broken fixture cannot silently remove the containment assertion." Both probes catch bare OSError and return False.

  This is correct and it is the strongest kind of finding — the paragraph contradicted its own commit. test/conftest.py:88 and conftest.py:123 both catch bare OSError and return False, this diff's own test_an_unanticipated_oserror_still_only_disables_the_capability pins EIO → False without propagating, and the PR description says the probe "keeps the fail-soft contract it already has on main". Three sources agreed with each other and the paragraph disagreed with all of them. Left in, it would have instructed the next contributor to build the errno allowlist this change explicitly argues against. Deleting costs nothing: the 501 removal and the 503 translation stand without it.

• Watch — the sibling probe's contract stays unpinned: accepted-and-deferred → filed Pin the rootdir symlink probe's fail-soft contract (sibling of the probe #5943 pinned) #5946.

  The same fail-soft contract exists in two probes — _can_create_symlink (test/conftest.py:67) and _root_can_create_real_symlink (conftest.py:106); the new tests pin only the first.

  Agreed on both halves: the gap is real, and nothing is live in it — both probes already behave identically, so there is no defect today, only an unpinned invariant that a future edit could silently break. Pinning the rootdir sibling is test-infra work that stands entirely on its own and shares no file with this change, so folding it in here would widen a PR that is already a cross-fork rebase carrying two review fixes. Pin the rootdir symlink probe's fail-soft contract (sibling of the probe #5943 pinned) #5946 names the concrete task.

Also confirming the two "rides along, declared" items the review flagged rather than blocked, since both are called out in the PR body: the new probe test file, and the error-code-baseline.json regeneration whose _compliant 1097 → 1154 movement is pre-existing staleness (only the three totals lines changed; the per-file map is byte-identical).

[iamwhatever]

Disposition for the GPT BLOCKING finding on 9db1a531912a:

• Windows installs expose UNC credential probes (themes.py:534): fixed in c8631e0b1.

  \\attacker\share -> install API -> _resolve_local_source -> Path.is_dir() initiates SMB authentication -> gateway credentials reach the attacker

  Legitimate, reachable, and the most serious finding on this PR. It is also already a settled invariant in this repository, which is what removes any doubt: hooks.unc_probe_allowed exists for precisely this hazard and documents it in the same terms — "a UNC path names a HOST, so resolving or stat-ing untrusted text … makes Windows open an SMB connection to that host — an outbound credential probe the attacker controls" — and acp/prompt_blocks.py:171 already applies it to attachment paths. docs/guides/windows-install.md records the same reasoning for project skills. _resolve_local_source was the surface that had not adopted it, and lifting the 501 made it reachable.

  Span note — this is hit 2 of the same span. Round 1 was a junction cycle in _copy_installed_theme; this is UNC in _resolve_local_source. Both are the same root cause: the install path performed filesystem access on user-supplied text before validating its shape, using primitives that are wrong on Windows. Per the loop's own recurrence rule I stopped point-fixing and did a restructure round rather than patch UNC and wait for a third sibling.

  The invariant, and the table of branches it covers. Every filesystem call in _resolve_local_source is now preceded by shape validation, screened in this order:

  branch	primitive that was wrong	now
  UNC host path (\\host\share, //host/share)	any stat = the SMB probe itself	lexical is_unc_shape + unc_probe_allowed before any filesystem call; expanduser only reads the environment
  pack root is a junction	Path.is_symlink() is False for a junction	is_link_or_junction
  pack subdirectory is a junction	os.path.islink is False for a junction	is_link_or_junction (round 1, _copy_installed_theme)
  pack root is a POSIX symlink	already correct	unchanged
  sensitive location	already correct	unchanged, still on the resolved path

  I again did not take the suggested fix, for the same reason as round 1:

  Fix: Restore the Windows install guard until UNC paths and root junctions are rejected before filesystem access.

  Restoring the 501 reverts the PR's purpose; the subordinate clause names the real condition, and both halves of it are now met. Note the screen is not a blanket UNC ban: unc_probe_allowed keeps the one legitimate case working — a roaming profile whose home directory is itself a UNC share — which a flat reject would have broken.

  Tests. Three added, and the first one asserts the security property rather than the symptom: test_a_unc_source_is_refused_without_touching_the_filesystem wires both Path.is_dir and is_link_or_junction to raise, so any filesystem access on a UNC path fails the test — refusing with the right message is not enough, the stat must never happen. test_a_roaming_profile_unc_source_gets_past_the_shape_screen is its exact inverse (is_dir must be reached), guarding against a fix that over-blocks. test_a_junction_root_is_refused covers the root case.

  One honest note on those tests: my first version of the roaming-profile test passed a real POSIX tmp_path, which is not UNC-shaped, so the screen never fired and it asserted nothing about the branch it named. It failed for that reason and was rewritten to drive a genuinely UNC-shaped input.

Gates after the fix: 615 passed / 1 skipped, isort / flake8 / mypy clean.

[iamwhatever]

Disposition for the GPT BLOCKING finding on c8631e0b15b6:

• Removing the Windows guard exposes synchronous UNC resolution on the event loop (themes.py:765): fixed in 1543104fe.

  UNC-backed Windows data home -> asset/overlay/topbar request -> synchronous SMB-backed resolve()/is_file() -> gateway event loop stalls.

  Legitimate. _resolve_theme_asset (theme_validate.py:1580) does resolve() and is_file(), and each of the three routes called it inline while already offloading only its read to discovery_executor(). On a UNC-backed data home those stats are SMB-backed, so the loop blocked on network I/O per request — and the 501 gate is what previously kept these routes off Windows entirely, so lifting it is what exposes it. It also contradicted this module's own stated invariant that these coroutines do no blocking filesystem work on the loop.

  Fixed by moving the resolve onto the executor the read already uses, at all three sites (api_theme_asset, api_theme_overlay, api_theme_topbar). Zero inline _resolve_theme_asset calls remain. Again not by restoring the gate — that is the third round this lane has proposed reverting the PR's purpose as the fix, and each time the finding underneath it was real while the remedy was not.

Span ledger — hit 3, and why I am flagging rather than just patching

Same file, three rounds, each closing exactly the finding it was given and receiving one new sibling:

round	head	finding	root cause
1	8d841b7a6b3e	junction cycle in _copy_installed_theme	wrong primitive: os.path.islink is False for a junction
2	9db1a531912a	UNC credential probe in _resolve_local_source	filesystem access before shape validation
3	c8631e0b15b6	synchronous UNC resolve on the loop in the serve routes	blocking call on the event loop

Rounds 1–2 shared one root cause, and round 2 answered them together with an invariant plus a branch table rather than a point fix. Round 3 is a genuinely different invariant (event-loop offload, not input validation), which is why it was not caught by that restructure.

The honest read of the pattern: the 501 gate was hiding a class of Windows-specific hazards, not one bug, and each round surfaces the next member. That is worth a maintainer's eye — the code is better after each round, but "one more round will be the last" has now been wrong three times. I am flagging it here rather than silently continuing so the next reviewer can see the count.

Also in 1543104fe, and it was my own regression

Backend Tests (3.10, 2) died with INTERNALERROR ... NotImplementedError: cannot instantiate 'WindowsPath' on your system, aborting the entire shard rather than failing one test. Cause: my round-2 test patched os.name to "nt" process-wide, which makes pathlib construct a WindowsPath on Linux. Fixed by reading the repo's existing platform_compat.IS_WINDOWS in the production guard and having the tests flip that module-level binding instead — no lying to pathlib about the platform, and the production code is more idiomatic for it. Coverage Gate was downstream of the same aborted shard.

UX Review was an infrastructure failure, not a code signal — setup-bun could not be downloaded (Name or service not known (internal-api.service.iad.github.net:443)). Re-run, no diff change.

Gates on 1543104fe: 624 passed, isort / flake8 / mypy clean.

[iamwhatever]

Dispositions for the First Principles CONCERNS on 1543104fe468, one per finding:

• Watch 1 — the description claims "one failure it was hiding" while the diff hardens four more surfaces: fixed (description only, no code change, no push).

  the diff handles four undeclared ones (items 3–6). Items 3–5 are boundary-derived and belong here; the framing just undersells the surface a reviewer must check.

  Entirely correct, and the cause is a process miss of mine rather than a judgement call: the body was written before the first CI round and never reconciled after three rounds of fixes, which is exactly the step the drive-to-green loop requires after any amend that changes the diff. The body now enumerates all five consequence items (503 translation, UNC screen, junction root, junction subdirectory, event-loop offload), states that the gate was hiding a class of Windows hazards rather than one bug, and says plainly that the earlier "one failure" wording undersold the review surface.

• Watch 2 / Subtraction — item 6 is a point patch; six sibling stats remain on the loop in api_theme_detail: needs-a-decision, and I am putting the question to the maintainer rather than filing it, because the answer is a scope ruling only they can make.

  Item 6 … applies equally to the 6 inline target.exists() / dir_target.is_dir() calls still on the loop in api_theme_detail … Three call sites fixed, six siblings left.

  The finding holds and I am not going to argue it: the six siblings share item 6's exact rationale, and api_theme_detail was behind the same 501 gate, so they are newly reachable on Windows by the identical argument. "Fixed a third of the stall its own comment describes" is a fair characterisation.

  The literal subtraction is not available, though, and that is worth stating rather than quietly ignoring: dropping the three _resolve_theme_asset hunks would re-open the GPT blocking finding on c8631e0b15b6, which is what those hunks exist to answer. So the choice is not "drop them or keep them" — it is:

  1. Widen this PR to all nine sites. Complete, but it is a fourth round on a PR that is currently green across 66 checks, and it re-arms every reviewer on new surface — including a lane that has blocked three times.
  2. Land this green and do all nine together in a follow-up, which is the spirit of the subtraction (one change owning the whole offload) minus the part that would un-green the PR.

  I lean to (2) and have declared the gap in the PR body under "Known incomplete", with the specific handler named, so it cannot be discovered later as a surprise. But widening is defensible and I am not going to pick a scope for the maintainer — @iamwhatever, which do you want?

  Deliberately not filing an issue for this one: an issue whose body asks someone to choose between two scopes is not a task anybody can pick up, so it would sit in the tracker while the question went unread. If the answer is (2) I will file the follow-up then, naming all nine sites as concrete work.

Also confirming the items the review classified as rides-along, since all are now declared in the body: the probe test file, the _same case/path normalisation, and the error-code-baseline.json regeneration whose _compliant movement is pre-existing staleness (three totals lines changed, per-file map byte-identical).

[iamwhatever]

Correction to my note above, now that the round of checks on f86d1dfca has finished: GPT 5.6 did NOT re-raise the round-6 TOCTOU finding on this head. All 66 checks pass, PR Readiness is green, and all four reviewer lanes (GPT, Opus, Design, First Principles) report no blocking findings. No /ai-review override was posted and none was needed.

Being explicit about what that does and does not mean, since my comment above asked for a maintainer call and a green rollup could be misread as the call having been answered:

• The code is byte-identical on the security seam. This push changed an import line and a generated baseline. _resolve_local_source is unchanged.
• So the TOCTOU residual is still there. Every path check in this function is check-then-act, and a sufficiently-timed directory swap between the ancestor walk and is_dir() can still make the probe resolve a swapped junction. The round-6 disposition above stands as the written record of it; the lane simply did not raise it this time.
• What is still worth deciding, and is no longer blocking anything: whether handle-relative traversal (NtCreateFile + FILE_OPEN_REPARSE_POINT per component, as a platform_compat/hooks primitive every path surface can adopt) gets tracked as its own work. It is the only complete fix, it is Windows-primitives work rather than a theme-handler change, and the same race exists at the other path surfaces on main today. Say the word and I will file it alongside Ancestor-link seam remains at the non-themes is_unc_shape call sites #5962 / Move the remaining six api_theme_detail stats off the event loop #5963.

Nothing else is outstanding: mergeable, single commit, closes #311 resolved by GitHub, zero unresolved threads. Awaiting human approval only.

[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 #6305 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 #6305: CONTINUE_DEVELOPMENT. The merged PR supplies the helper and the reference pattern but covers none of the five call sites PR #6305 fixes; issue Issue #5962 was filed precisely to track that remainder and is still open. Files: src/kiro_crew/platform_compat.py.

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