feat(windows): answer the provider-CLI trust question from the ACL

[chenmingwei23]

Closes #4604.

Problem / Motivation

github_runner.validate_provider_executable is the single trust gate for every
gh / glab spawn. Its policy was written in POSIX ownership terms, so Windows
was refused at four call sites and Issue Radar could not be used at all there,
with gh installed and authenticated.

The refusal was the right call at the time, not laziness. Measured on Windows 11:

os.getuid / os.geteuid   absent
gh                       C:\Program Files\GitHub CLI\gh.EXE
st_uid                   0
st_mode                  0o100777      S_IWGRP=True  S_IWOTH=True

Both halves of the predicate degenerate, in opposite directions: st_uid is
always 0, which st_uid not in (0, uid) whitelists as root-owned, so the
ownership half always passes; st_mode is always 0o777, so the permission
half always refuses, flagging every file on the host as world-writable.
Neither result depends on the file examined.

Why it matters

A Windows user with a working, authenticated gh was told the feature does not
exist on their platform. The only documented remedy was to run the whole gateway
under WSL, which means a second install with its own data home and its own gh
login.

What changed (motivation → approach → change)

Answer the two questions the policy actually asks — is this component owned by a
third account, and can anything outside the trusted set replace it — from the
object's security descriptor.

The new windows_acl module reads the owner SID and the principals holding a
substitution-capable right, and deliberately holds no policy: keeping the
read separate from the decision is what lets the policy be tested against
synthetic ACLs on the Ubuntu runner rather than only on a Windows one. The
trusted set is {gateway user, S-1-5-18, S-1-5-32-544, TrustedInstaller} — the
direct analog of POSIX (0, uid).

Both token reads live in platform_compat, not in the new module, because it
already owns "read this process's own access token" for the codebase:
current_user_sid (memoised, already the single source of "who is the owner" for
the gateway's pipe DACLs) and a new is_token_elevated. Both are tri-state and
both non-True answers refuse — an unreadable token is not a not-elevated
token, and an unverifiable SID is not a trusted one. windows_acl keeps only the
ACL-read surface (GetNamedSecurityInfoW, GetAce, ConvertSidToStringSidW,
LookupAccountSidW), so no prototype is declared twice.

No new dependency. ctypes covers every call; pywin32 would have been a
first platform-conditional dependency for nothing.

The part worth reviewing closely

The POSIX predicate is not ported bit-for-bit, on purpose:

bit 0x2   FILE = FILE_WRITE_DATA     DIRECTORY = FILE_ADD_FILE
bit 0x4   FILE = FILE_APPEND_DATA    DIRECTORY = FILE_ADD_SUBDIRECTORY

POSIX collapses every directory mutation into one w bit, so "the parent is
writable" genuinely implies "the entry can be swapped" — unlink and rename come
with it. Windows decomposes them, and neither directory right can touch an
existing entry. Replacing one needs FILE_DELETE_CHILD on the parent, or
DELETE / WRITE_DAC / WRITE_OWNER on the entry itself.

This is load-bearing, not pedantic. The stock C:\ ACL grants
NT AUTHORITY\Authenticated Users the ADD_SUBDIRECTORY right, so a predicate
that reads that bit as a write grant refuses every default Windows install
once the walk reaches the drive root. The first draft of this change did exactly
that. test_windows_acl.py pins the distinction from both sides: the right is
ignored, the principal is not (Authenticated Users holding FILE_DELETE_CHILD
is still refused).

Strict mode and Windows path casing

validate_provider_executable's strict mode refuses a path that differs from its
own resolution, on the grounds that it is not canonical. On Windows that test
needed to become case-insensitive, and this is a behaviour change worth stating
rather than leaving in a code comment: paths there are case-insensitive and
Path.resolve() rewrites a component to its on-disk casing, so a candidate
spelled gh.exe against a file named gh.EXE differs from its resolution with
no symlink involved. Comparing case-sensitively would refuse a plain install in
strict mode and pointlessly re-walk the same parents in relaxed mode. The
mismatch is real rather than hypothetical: shutil.which returns the name cased
as PATHEXT spells it.

Discovery could never have found a Windows gh either

os.path.join(entry, "gh") never matches gh.exe, so the candidate scan
returned nothing on Windows regardless of the trust policy. Resolution inside a
directory is now delegated to shutil.which, which applies whatever the platform
defines as runnable there — PATHEXT on Windows, X_OK on POSIX — and the
Program Files install dirs lead the ambient PATH so a machine-wide install
still wins over a user-writable shim.

Delegating rather than hand-rolling this also removed a bug the first draft
carried: it split PATHEXT on os.pathsep, which is ; on Windows but :
elsewhere, conflating "how PATH is joined" with "how PATHEXT is joined".

Fail-closed decisions

• an unreadable security descriptor is a refusal, never "nothing found"
• a NULL DACL grants everyone full control, and is reported explicitly so an
  empty writers tuple cannot be mistaken for a clean result
• an ACE type this policy cannot parse (object / callback ACEs place the SID at a
  different offset) is a refusal, because the writers tuple would be incomplete
• deny ACEs are ignored, which is the conservative direction: it can name a
  writer that is in fact denied — refusing a binary that would have been fine —
  and never the reverse. A full allow/deny resolution would need AccessCheck(),
  which answers "may this token write" rather than the policy's question ("may
  anyone outside the trusted set write"), so a hand-walk is required here.
• an elevated gateway is refused for the same reason a root one is: its
  children would be elevated too and the whole walk goes vacuous
• an unverifiable gateway SID is a refusal

What this does NOT unblock, despite sharing the gate

The pull-request source drawer stays refused on Windows, and an earlier
revision of this body wrongly claimed otherwise. The drawer does not share Issue
Radar's spawn: it keeps its own async, sandbox-routed _run_json, which carries a
separate refusal at source_providers.py:713 phrased around the sandbox rather
than the trust check, and fail-closes where no OS sandbox backend exists. Fixing
the trust check therefore does not unblock it, and that guard is deliberately
left in place. windows-install.md now says so, and says which of the two
blockers applies to which surface.

Code Review Sage stays refused on Windows — deliberately

Sage shares the trust gate, so that half now passes for it too, but it needs a
second thing Issue Radar does not: its review prompts and its shipped
sage-review skill hand the worker session python3 sage_lib/… commands, and
python3 is not an interpreter on Windows — the name resolves to the Microsoft
Store app-execution alias, or to nothing. Enabling Sage would trade a clear
refusal for a review that starts and produces no result record, so its Windows
refusal is kept and reworded to name the actual blocker. Tracked as #4630, whose
acceptance criterion is removing that refusal.

Two stale test gates and one baseline prune

test/test_github_runner.py and the Sage conftest.py both skip on Windows;
their stated reasons are corrected in place so neither claims a refusal that no
longer exists. Neither suite is unskipped here — they assert the POSIX branch's
own messages and build #!/bin/sh stubs, which is separate work.

.github/black-baseline.txt loses four entries. The black gate is a shrinking
baseline and reports any baselined file that has become clean; three of the four
(cli_commands.py, memory.py, sandbox.py) graduated through recent commits on
main rather than through this diff, and whichever PR rebases first is the one
that has to prune them.

Provider output is now decoded as UTF-8, on every platform

Removing the Windows refusals exposed a defect the refusals had been hiding, so
this is in scope as a bug in this diff rather than adjacent work — but it changes
behaviour on POSIX too, and it is named here rather than left to be found in
the diff.

run_gh and _glab_run passed text=True with no encoding=, which decodes with
the locale codec — the ANSI codepage on Windows. Reproduced on a cp936 host:
UnicodeDecodeError: 'gbk' codec can't decode byte 0xac in position 27, raised
inside subprocess's own reader thread, so subprocess.run returns with
stdout=None and the caller dies on the None instead of on a decode error
anyone could attribute.

Both chokepoints now capture bytes and decode strictly in their own frame.
Two rejected alternatives, since the choice is not obvious:

• encoding="utf-8" alone fixes the codec but not the attribution — a strict
  failure still dies on that reader thread, handing the caller stdout=None.
• errors="replace", which an earlier revision of this PR used, is worse than it
  looks. U+FFFD inside a JSON string value leaves the document syntactically
  valid, so json.loads succeeds and the replacement character flows on into
  stored issue records. That is silent corruption, not a handled error.

Decoding in our own frame keeps both properties: strict, so nothing is silently
corrupted, and attributable, so the failure names the stream and byte offset
without echoing payload bytes.

Tests

test/test_windows_acl.py (new). Two layers, on purpose:

• the policy in check_provider_path_component_windows, driven by synthetic
  ComponentSecurity values so it runs on every runner including the Ubuntu one
  that gates CI — trusted and untrusted owners, untrusted writers, NULL DACL,
  unparsable ACE types, unreadable descriptors, strict vs relaxed, and the
  ADD_SUBDIRECTORY carve-out from both sides;
• the ACL read in windows_acl.describe, Windows-only because it needs a
  real security descriptor: a user-owned tree, a synthetic
  icacls /grant "Everyone:(OI)(CI)F" tree that must be refused, and the drive
  root that must not refuse the walk.

Candidate discovery is covered by asserting the delegation (a PATH hit resolves
absolute; strict mode ignores PATH; on Windows a bare name resolves to .exe).

Manual verification

On Windows 11, AMD64, non-elevated:

• resolve_gh() → C:\Program Files\GitHub CLI\gh.exe
• Issue Radar _gh_bin() succeeds; Sage gh_bin() refuses with the interpreter
  reason
• a real gh api user completes through run_gh (rc 0)
• flake8, isort and the repo's own scripts/check_black_formatting.py pass on
  the full CI scope
• mypy --platform linux src/kiro_crew/ — the exact command CI's Backend Lint
  runs on Ubuntu — reports Success: no issues found in 1000 source files. Bare
  mypy on Windows cannot substitute: it defaults --platform to the host, so it
  analyses the wintypes branch and never sees the fallback branch CI checks.
• 414 passed, 64 skipped, 0 failed across the four touched suites

test/test_source_providers.py fails heavily on Windows before this change
(baseline 20 passed / 16 failed then an asyncio hang; with this change 21 passed
/ 15 failed then the same hang). Pre-existing Windows breakage, one failure
fewer, and out of scope here.

Not covered by host state, and better served by synthetic fixtures than by
whatever the runner happens to have installed: winget and scoop shim layouts,
GPO-altered Program Files ACLs, UNC paths and mapped drives, ARM64.

Screenshots / video

Why no screenshot: backend trust-policy and docs change only; no frontend
path is touched and nothing renders differently.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 16290a1

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Read-only ACL facts separated from a fail-closed policy is the right shape; every unknown (unreadable descriptor, alien ACE type, remote volume, elevation) refuses rather than trusts.

[DESIGN-REVIEWED] 16290a1

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 16290a145afa577fccb4070d7ecffcf568af70e0 — 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 repo around the change to verify counts before writing the review: I checked consumers of windows_acl, is_token_elevated, and _DRIVE_REMOTE, and grepped for sibling gh-output decode sites. Findings verified; emitting the review.

First-Principles-Verdict: CONCERNS

Every item traces to the reported defect, but the strict-UTF-8 fix leaves its own named corruption alive in a sibling gh runner, and one dead constant ships.

What this change ships

Intent: let a Windows user with a working, authenticated gh use Issue Radar (#4604). FIX.

1. Issue Radar works on Windows — the fix; three platform refusals deleted — justified
2. Binary trust answered from the file's ACL (windows_acl + one policy function) — justified, cause-level
3. Elevated or unreadable-token gateway refused — justified (analog of the root refusal)
4. Candidate scan now finds gh.exe (delegates to shutil.which, Program Files dirs first) — justified; scan previously returned nothing
5. Strict mode accepts case-differing paths on Windows — declared, derived from Path.resolve() casing rewrite
6. gh/glab output decoded strictly as UTF-8 on all platforms — declared; one unfixed sibling
7. Sage stays refused; message now names python3, not POSIX — justified correction of a now-false claim
8. PR source drawer stays refused; docs name which blocker applies where — justified
9. Test stubs hand bytes; two stale skip reasons corrected — rides with item 6, declared
10. Specs/docs updated in the same commit — mandated by AGENTS.md

Watch

• Item 6's root cause — replacement characters flowing into stored records — has 1 counted unfixed sibling: ops_mission_control/backend/providers/github_issues.py:98 decodes gh output with errors="replace" into signal records, exactly the corruption test_replacing_bad_bytes_would_corrupt_a_json_string_value pins as "the problem" (grepped errors="replace" against gh stdout paths). It has its own spawn, so out of this PR's chokepoints — accepted-and-deferred, but say so somewhere a fixer will find it (Code Review Sage's review worker hardcodes python3, which is not an interpreter on Windows #4630-style tracking, or fix it).

Subtractions

• Delete _DRIVE_REMOTE (src/kiro_crew/windows_acl.py:128) — 0 consumers (grep _DRIVE_REMOTE: 1 hit, the defining site); _LOCAL_DRIVE_TYPES is what the check consults.

[FIRST-PRINCIPLES-REVIEWED] 16290a1

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've analyzed all three candidates against the diff and surrounding code.

Candidate 1 (_inside drops a hit at a drive root): The only way directory is a bare drive root (C:\) is a PATH entry literally equal to C:\ with gh.exe sitting directly at the root — _wellknown_windows_dirs only ever yields Program Files\... subdirs. That input does not occur in practice, and the outcome is a fail-closed over-refusal (a missed install), harming no boundary. Cannot establish (a) or a harmful (c). Drop.

Candidate 2 (\\?\ extended-length prefix false-refuse in strict mode): the candidate itself cannot establish that Path.resolve yields the extended form for ordinary paths, and CPython normally strips it. No concrete input, and again fail-closed over-refusal. Drop.

Candidate 3 (dropping text=True preserves CRLF): every consumer opened uses splitlines()/json.loads, both CRLF-tolerant. run_gh/_glab_run still return str (decoded manually), so no type regression. No observable wrong outcome establishable. Drop.

No self-originated finding rises to the 80+ bar: the ACL parse fails closed on NULL DACL, unparsable ACE types, unreadable descriptors, and remote volumes; the _inside containment correctly drops relative CWD hits; elevation and SID reads are tri-state with both non-True answers refusing.

No findings.

[OPUS-REVIEWED] 16290a1

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

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

[chenmingwei23]

GPT 5.6 disposition — ef0b79911

• discovery.py:111 — Sage is enabled before its worker commands support Windows

  Windows desktop bundle ships only python.exe -> Sage review prompt invokes python3 sage_lib/... -> command fails and no review record is produced. Fix: Restore the Windows refusal until Sage invokes a Windows-compatible interpreter.

  fixed — the Windows refusal is restored in sage_lib/discovery.py, exactly as
  requested, and its message now names the real remaining blocker instead of the
  trust check that no longer applies.

  Verified on a Windows 11 host rather than assumed: python3.exe is absent from
  both venvs (.venv/Scripts, .venv-test/Scripts), and where python3 resolves
  only to %LOCALAPPDATA%\Microsoft\WindowsApps\python3.exe — the Store
  app-execution alias, not an interpreter. The name is hardcoded in five prompt
  strings in review_driver.py and three commands in the shipped
  skills/sage-review/SKILL.md, so it is a separable defect with its own surface,
  now tracked as Code Review Sage's review worker hardcodes python3, which is not an interpreter on Windows #4630 with the acceptance criterion being removal of this
  refusal.

  Issue Radar is unaffected and does work on Windows — that split is now stated in
  the refusal message, the PR body, and the windows-install.md support table, so
  a Windows operator is told which of the two they have.

[chenmingwei23]

Design Review disposition — ef0b79911

• Spec rot the repo forbids

  docs/system-specs/modules/issue-radar.md:951 still says "POSIX only (macOS/Linux). Windows raises GhCliError immediately," and the docs/guides/windows-install.md support matrix still lists provider-CLI features as unavailable — both made false by this diff. AGENTS.md mandates spec updates in the SAME commit.

  fixed — both are in this commit. The issue-radar spec's Platform
  Requirements now state that the trust check is answered from POSIX ownership or
  from the ACL, name the elevated-gateway refusal, and add the Program Files
  search dirs alongside the POSIX well-known dirs. windows-install.md's
  per-feature table replaces the "not yet — provider CLIs require the POSIX
  OS-level sandbox" row (whose stated reason was also wrong: the blocker was the
  ownership check, never the sandbox) and adds explicit Issue Radar and Code
  Review Sage rows, because those two now differ from each other on this platform
  and a reader needs to be told which.

• The riskiest layer has no CI execution path

  The hand-rolled ctypes ACE walk is tested only in the Windows-only half of test_windows_acl.py, and no workflow runs pytest on a Windows runner. Until a Windows test lane exists, regressions in the read layer ship on one manual verification.

  needs-a-decision — the observation is exactly right and I am not going to
  pretend otherwise: the ACE walk's real-descriptor tests do not execute anywhere
  in CI, so today they are worth only the one manual run behind them.

  Two things bound the exposure in the meantime, and neither substitutes for the
  lane. First, the policy layer is covered on the Ubuntu runner: windows_acl
  holds no policy, so every decision — trusted/untrusted owner, untrusted writer,
  NULL DACL, unparsable ACE, unreadable descriptor, strict vs relaxed, and the
  ADD_SUBDIRECTORY carve-out that would otherwise refuse every default Windows
  install — is driven from synthetic ComponentSecurity values and runs on every
  PR. What is uncovered is specifically the read: struct offsets and SID pointer
  math. Second, the read fails closed, so a regression there costs availability
  (an over-refusal with a message naming the offender), not trust.

  What I cannot decide is whether this repo wants to start paying for a Windows
  pytest lane, since that is runner cost and maintenance you own rather than a
  code change I can justify inside this PR. Which would you prefer: a Windows
  job scoped to just the platform-specific suites, a full Windows matrix entry, or
  leaving it uncovered with the fail-closed argument on the record? I have
  deliberately not filed an issue for this — it is a question, and an issue that
  asks a maintainer to choose is not a task anyone can pick up.

[chenmingwei23]

First Principles disposition — ef0b79911

• Duplication: windows_acl.current_user_sid re-implements platform_compat.current_user_sid

  platform_compat.py:3005 already reads the token SID via the same OpenProcessToken/GetTokenInformation/ConvertSidToStringSidW calls, memoised, with 3 consumers. transport.py:211's comment names it the single source of "who is the owner". The only delta is None-vs-raise.

  fixed — windows_acl.current_user_sid is deleted, along with the
  _TOKEN_USER / _SID_AND_ATTRIBUTES structs and the TokenUser class
  constant that existed only to serve it. validate_provider_executable now calls
  platform_compat.current_user_sid() and treats None the way every other
  consumer does — principal unverifiable, therefore refuse. The OpenProcessToken
  and GetTokenInformation prototypes stay in _load, because
  is_token_elevated still needs them for TokenElevation; that one has no
  equivalent in platform_compat.

  Worth noting the second-order gain: platform_compat's version is memoised, so
  the per-component walk no longer re-reads the token once per candidate.

• Subtraction: replace _windows_executable_names and the per-name join loops with shutil.which

  the stdlib applies PATHEXT and X_OK; the repo already uses this pattern (transcribe.py:84, perf_sampler.py:391).

  fixed — and this one caught a live bug rather than only shortening the code,
  so it is worth recording why. _windows_executable_names split PATHEXT on
  os.pathsep, which is ; on Windows but : elsewhere; CI's Linux runner
  therefore got one unsplit blob and two of the new tests failed on it. The
  underlying mistake is conflating "how PATH is joined" with "how PATHEXT is
  joined" — they merely coincide on Windows. Delegating to shutil.which removes
  the hand-rolled table, the per-name join loops, _windows_executable_names
  itself, and the separate os.access(X_OK) check, and it cannot get the
  separator wrong. _wellknown_windows_dirs now returns only directories and
  shutil.which(executable, path=directory) resolves inside each one, which is
  also how the POSIX PATH scan is now expressed — one code path for both
  platforms instead of two.

  Net effect on the diff: github_runner.py is 79 changed lines instead of 110,
  and windows_acl.py lost 32.

• The same_path casefold comparison changes strict-mode acceptance on Windows and is declared nowhere a human deciding on the PR reads

  fixed — the PR body now states it under a heading of its own, with the
  reason: Windows paths are case-insensitive and Path.resolve() rewrites a
  component to its on-disk casing, so a candidate spelled gh.exe against a file
  named gh.EXE differs from its resolution with no symlink involved, and
  comparing case-sensitively there would refuse a plain install in strict mode.
  This is no longer a hypothetical: shutil.which returns the name cased as
  PATHEXT spells it (gh.EXE), which is exactly the mismatch, and a test now
  pins that reasoning where the behaviour is asserted.

• Item 3 in "What this change ships": "hand-rolls what shutil.which does"

  fixed — same change as the subtraction above; the hand-rolled resolution is
  gone.

[chenmingwei23]

Design Review disposition — correction to my previous reply

I answered the CI-coverage concern as needs-a-decision and asked which shape of
Windows test lane this repo wanted. That was wrong, and the premise it rested on
is false. Correcting it rather than leaving a question standing that has already
been answered by the repo.

• The riskiest layer has no CI execution path

  The hand-rolled ctypes ACE walk (windows_acl.describe — manual struct offsets, SidStart pointer math) is tested only in the Windows-only half of test_windows_acl.py, and no workflow runs pytest on a Windows runner (build-windows.yml is installer-only, cross-platform.yml is ubuntu-only).

  rebutted — a Windows pytest lane already exists, in ci.yml itself:

  ci.yml:555   backend-test-windows:
  ci.yml:556     name: Backend Tests (Windows)
  ci.yml:558     runs-on: windows-latest
                 python-version: "3.12", matrix group [1,2,3,4]
  ci.yml:630     pytest -q -n auto --timeout=180 --no-cov \
  ci.yml:631       --splits "$SHARD_COUNT" --group ${{ matrix.group }}
  

  The two workflows the finding checked are the right ones to rule out, but the
  lane is not in either of them — it is a job in the main CI workflow, whose own
  comment states the intent: "until this job existed nothing in CI held that line
  — a POSIX-only regression would land silently." There is no whole-file
  deselect on that job (residual POSIX assumptions are tracked as node ids in
  test/windows-expected-failures.txt instead), so test/test_windows_acl.py
  runs there in full, including the windows_only class that exercises the real
  security descriptor: the user-owned tree, the synthetic
  icacls /grant "Everyone:(OI)(CI)F" refusal, and the drive-root walk.

  So the ACE walk's struct offsets and SID pointer math are covered by four
  Backend Tests (Windows) shards on every push, not by one manual run. The
  concern's conclusion — that a regression in the read layer would surface as user
  bug reports rather than red CI — does not hold.

  What remains true from the finding, and worth keeping on the record: the walk is
  exercised only against the ACLs a GitHub-hosted windows-latest runner happens
  to have. A GPO-managed or domain-joined host with a rewritten
  Program Files ACL is still untested, which the PR body already lists as
  uncovered. That is a narrower gap than "no CI execution path", and it is bounded
  the same way: the read fails closed, so the failure mode is an over-refusal that
  names the offending principal, not silent trust.

Withdrawing the question I put to the maintainer in the previous comment — it does
not need answering.

[chenmingwei23]

CI disposition — e6ae3c675

Two root causes behind four red checks on ef0b79911. Both were real defects in
this diff, and both are structurally invisible to a Windows-only local gate —
worth naming, because that is the gap that let them through.

• Backend Lint & Type Check (3.12 failed, 3.10 cancelled with it) — mypy src/kiro_crew/

  fixed — ctypes.WinDLL and ctypes.get_last_error are Windows-only in
  typeshed. mypy defaults --platform to the host, so on Windows it analysed the
  from ctypes import wintypes branch and never looked at the non-Windows one; on
  CI's Ubuntu runner it analyses the fallback branch and reports 7 errors —
  3 × Name "C.WinDLL" is not defined, 4 × Module has no attribute "get_last_error".

  Fixed with two named shims in windows_acl.py rather than per-line ignores: a
  _DLL = Any alias for the loaded-library annotations (opaque handles this module
  never introspects) and a _last_error() helper that reaches the symbol
  opaquely. _load still refuses off Windows before any of it can run, so nothing
  is weakened.

  Reproduced and verified locally with mypy --platform linux, which is the
  command a Windows contributor needs for this file and is now the only way to
  check it honestly from here. Clean on both platforms:

  mypy --platform linux src/kiro_crew/windows_acl.py src/kiro_crew/github_runner.py
    Success: no issues found in 2 source files
  mypy               src/kiro_crew/windows_acl.py src/kiro_crew/github_runner.py
    Success: no issues found in 2 source files
  

• Backend Tests (3.10 shard 4, 3.12 shard 4) — test_wellknown_windows_dirs

  fixed — the test monkeypatched sys.platform to win32 but asserted
  literal backslash paths. On the Ubuntu runner os.path.join is still
  posixpath, so the function correctly returned C:\PF/GitHub CLI and the
  assertion described the runner's path flavour instead of the function's
  behaviour. The expectation is now built with os.path.join too, and the test
  says why in its docstring.

• Coverage Gate

  fixed — derivative: the combine step had no artifact from the two failed
  shards. No separate change.

Local gates on e6ae3c675, run as ci.yml runs them (full scope, not just the
changed files): scripts/check_black_formatting.py pass,
isort --check-only src/kiro_crew test conftest.py xdist_budget.py pass,
flake8 src/kiro_crew test conftest.py xdist_budget.py pass, mypy clean on both
platforms for the changed modules, 245 passed / 32 skipped / 0 failed across the
three touched suites.

[chenmingwei23]

GPT 5.6 disposition — 1d8f7d638

• github_client.py:111 — Windows reaches locale-decoded provider output

  Windows ANSI locale + Japanese issue title -> route -> _gh_run/_glab_run with text=True -> UnicodeDecodeError escapes as HTTP 500. Anchor: residual/crash-data-loss-corruption. Fix: Restore the Windows guards in both changed client hunks.

  fixed — the finding is correct, it is a real reachable crash, and removing
  the guard is what exposed it. Reproduced on this host, whose ANSI codepage
  is cp936, before any fix:

  UnicodeDecodeError: 'gbk' codec can't decode byte 0xac in position 27:
      illegal multibyte sequence
    File ".../subprocess.py", line 1597, in _readerthread
  

  One detail worth adding, because it makes the consequence worse than described:
  the decode happens on subprocess's own reader thread, so subprocess.run
  returns normally with stdout=None and the caller dies later on the None
  (TypeError: 'NoneType' object is not subscriptable) rather than on a decode
  error anyone could attribute to a codec. A 500 with an unrelated traceback is
  the observable.

  The fix is the codec, not the guard. gh and glab emit UTF-8 on every
  platform, so encoding="utf-8" is pinned at the two spawn chokepoints —
  github_runner.run_gh and gitlab_client._glab_run. Restoring the Windows
  refusal would re-hide the defect rather than fix it, and would also revert the
  feature this PR exists to deliver; and the bug is not actually Windows-specific
  in principle, since any non-UTF-8 locale reaches it. So the narrower fix would
  also have left POSIX exposed.

  errors="replace" rather than strict, deliberately: a strict failure
  reproduces exactly the unattributable stdout=None crash above, whereas U+FFFD
  makes json.loads raise JSONDecodeError, which every caller's existing error
  taxonomy already handles as a provider failure.

  There are two failure modes and the codepage picks which, so both are now
  pinned by a test: a byte the codepage rejects raises (the cp936 case above),
  while a sequence it accepts decodes to mojibake with no exception at all —
  the quieter half, and the one that would have shipped corrupted issue titles.
  My first draft of that test asserted only the raise and failed, because GBK is
  a wide multibyte codec that accepts the bytes I picked; it now asserts what the
  two modes share (the text does not survive).

  Precisely what is and is not verified, so the evidence is not overstated:
  verified that the pre-fix crash is real on a cp936 host, that the chokepoint
  now passes encoding="utf-8" (asserted by a unit test, not by inspection), and
  that real gh api calls return rc 0 with non-None stdout under cp936. NOT
  verified end to end: a CJK payload round-tripping through live gh, because the
  search call I used to fetch one returned no matching item. The mechanism and the
  codec pin are covered by tests; that last mile is not.

Local gates on 1d8f7d638, run as ci.yml runs them: black baseline gate pass,
isort / flake8 pass on the full CI scope, mypy --platform linux src/kiro_crew/ → Success: no issues found in 1000 source files, and 416 passed
/ 70 skipped / 0 failed across the five touched suites.

[chenmingwei23]

GPT 5.6 disposition — 2ed21d0ca

• github_runner.py:246 — the untrusted agent shares the SID treated as a trusted binary writer

  Prompt-injected content -> unsandboxed Windows agent replaces a per-user gh.exe -> Issue Radar executes it with forge credentials -> credential exposure. Anchor: residual/security. Fix: Do not trust me_sid for Windows provider binaries; require system-controlled components or retain the Windows refusal.

  rebutted — the threat is real, but it is not introduced by this diff and not
  Windows-specific: trusted.add(me_sid) is the exact analog of the POSIX rule
  this repository already ships, so removing it on Windows alone would not close
  the hole, it would only make one platform stricter than the other for an
  identical exposure.

  The relaxed POSIX predicate, unchanged by this PR, is at
  github_runner.py:196:

  # Relaxed policy: the gateway user's own installs are fine; a binary owned
  # by a third account or writable by the whole host is not.
  if path_stat.st_uid not in (0, uid):
      raise ValueError(f"{label} is owned by another user (uid {path_stat.st_uid})")

  uid there is geteuid() — the gateway user. So on Linux and macOS today a
  user-owned ~/.local/bin/gh, or a Homebrew gh under a user-writable prefix,
  is trusted and executed with the same forge credentials, reachable by the same
  prompt-injection chain. me_sid is that same rule expressed in ACL terms. If
  the consequence chain holds, it holds against main on every platform, and this
  PR is not where it starts.

  Both halves of the requested fix have a cost that is not paid back:

  • "require system-controlled components" means forcing strict mode on Windows
    only. Windows gh is installed per-user by both winget and scoop
    (%LOCALAPPDATA%), so this refuses the majority of real installs while a
    POSIX per-user install keeps working — an asymmetry with no security
    justification, since the threat is identical.
  • "retain the Windows refusal" is reverting the PR.

  The operator recourse already exists and is symmetric:
  KIROCREW_PROVIDER_BIN_STRICT=1 (github_runner.py:283) selects the strict
  branch on both platforms, which requires system ownership and refuses a
  component the gateway user can write. That is the documented control for exactly
  the multi-tenant / untrusted-agent posture this finding describes, and this PR
  implements it for Windows rather than leaving Windows with no strict mode at
  all. Two other fail-closed properties narrow the same surface further: an
  elevated gateway is refused outright, and an unverifiable SID refuses rather
  than degrading.

  Tightening the relaxed default to distrust the gateway user is a coherent
  proposal, but it is a cross-platform policy change to a pre-existing rule,
  affecting every POSIX install too — not a defect in this diff, and not something
  to land inside a PR whose subject is answering the same question on Windows. If
  you want that argued on its merits I will file it against the POSIX predicate
  where it actually originates; say so and I will.

Local gates on 2ed21d0ca (rebased onto ffb83d0577), run as ci.yml runs them:
black baseline gate pass, isort / flake8 pass on the full CI scope,
mypy --platform linux src/kiro_crew/ → Success: no issues found in 1000 source files, 419 passed / 70 skipped / 0 failed across the five touched suites.

[chenmingwei23]

Design Review disposition — 2ed21d0ca

• The cross-platform subprocess-decoding change in run_gh / _glab_run is undocumented scope

  this changes decode behavior on every platform (POSIX included: a bad byte that previously raised in the reader thread is now silently replaced with U+FFFD and can flow into stored issue records), yet the PR body never mentions it and no test in this diff covers it.

  fixed, and the concern was more right than the disposition I posted last
  round. I had argued that errors="replace" was safe because U+FFFD would make
  json.loads fail and land in the caller's existing error taxonomy. That holds
  only when the bad byte breaks JSON syntax. Inside a string value the
  document stays valid, json.loads succeeds, and the replacement character
  reaches the stored record — which is what this concern says, and my reasoning
  did not cover it. The consumers are github_client.py:189 and :251, both of
  which parse and store whatever comes back.

  So errors="replace" is gone. Both chokepoints now capture bytes and decode
  strictly in their own frame:

  • strict, so nothing is silently corrupted; and
  • in our frame rather than subprocess's, which is the reason I avoided strict in
    the first place — a strict failure inside subprocess's reader thread returns
    stdout=None and the caller dies on the None, with nothing naming the
    codec. SetupError / ProviderCliError now carry the stream and byte offset,
    and deliberately not the offending bytes, which are provider payload.

  On the two specific gaps named:

  • Described. The PR body has a new section, "Provider output is now decoded
    as UTF-8, on every platform", which states plainly that this changes POSIX
    behaviour, why it is in scope (removing the Windows refusals is what exposed
    it, so it is a defect in this diff rather than adjacent work), and both
    rejected alternatives with the reason each was rejected.
  • Covered. Four tests, not one: the chokepoint must not let subprocess do
    the decoding; an undecodable stream raises an attributable error and echoes no
    payload bytes; the same on the glab side; and one that pins the corruption
    path this concern identified — it asserts that errors="replace" yields a
    JSON document json.loads accepts with U+FFFD inside the parsed value, so the
    reasoning cannot quietly come back.

  On the "or split it out" half: I kept it here rather than splitting. The bug is
  only reachable because this PR removes the refusals that hid it, so landing the
  removal without the decode fix would ship a known crash on the very platform the
  PR exists to enable. Splitting would mean either merging that window or ordering
  two PRs to avoid it, and the fix is a dozen lines at two chokepoints. Naming it
  in the description was the right half of the request; splitting was not.

[chenmingwei23]

GPT 5.6 disposition — ab966ccc8

• github_runner.py:418 — Windows searches the current directory before the requested directory

  found = shutil.which(executable, path=directory) — Untrusted checkout containing gh.exe -> Issue Radar resolution -> attacker binary executes with provider credentials. Anchor: residual/security. Fix: Call shutil.which(os.path.join(directory, executable)) to retain PATHEXT handling without current-directory search.

  fixed — the finding is correct and it is my regression: this call arrived
  when I took the First Principles subtraction that replaced hand-rolled
  PATHEXT parsing with shutil.which, and the delegation brought CPython's
  Windows-only CWD precedence along with it. From shutil.which itself:

  if sys.platform == "win32":
      # The current directory takes precedence on Windows.
      curdir = os.curdir
      ...
      if curdir not in path:
          path.insert(0, curdir)

  Reproduced with an attacker copy in the CWD and the legitimate copy in the
  requested directory: which("gh.exe", path=<requested>) returned .\gh.exe
  — the CWD one. The relaxed trust policy then accepts it, because a checkout is
  owned by the gateway user.

  The fix is not the one suggested, because that form silently drops
  PATHEXT — and PATHEXT is the entire reason this code calls which at all.
  Measured all four combinations on Windows:

  call	result
  which("gh.exe", path=requested)	.\gh.exe — attacker CWD
  which(join(requested, "gh.exe"))	requested dir — CWD avoided
  which(join(requested, "gh"))	None — PATHEXT lost
  which("gh", path=requested)	.\gh.EXE — CWD, but PATHEXT applied

  os.path.join puts a dirname on cmd, which sends which down its
  early-return branch and past the PATHEXT loop, so a bare gh stops resolving
  to gh.exe. Taking that fix as written would trade a security bug for the
  functional regression that this PR exists to repair — the hand-rolled scan it
  replaced is the thing that "found nothing at all on Windows".

  So which keeps doing the PATHEXT work with path=directory, and the hit is
  then required to lie inside the directory that was asked for. The check uses
  abspath, deliberately not resolve: the only question here is whether the hit
  came from the requested directory, while where a symlink ultimately points
  belongs to the trust walk, which resolves and re-checks every component under
  its own policy. Three tests pin it — an outside hit is dropped, an inside hit is
  still kept (so the guard cannot pass by refusing everything), and the literal
  .\gh shape a real CWD hit returns is normalised before comparing.

  Those tests live in test/test_windows_acl.py, not next to the code in
  test/test_github_runner.py, and that placement is the point: the latter is
  skipped wholesale on Windows, so anything put there cannot be run on the only
  platform whose behaviour this is. They stub which rather than reading the host
  platform, so the Linux shards exercise the Windows hazard too.

• CI: TestRunGh — 5 failures, AttributeError: 'str' object has no attribute 'decode'

  fixed — mine, and the interesting part is why a green local run missed it.
  Last round moved both spawn chokepoints to capture bytes; test_github_runner.py's
  _proc helper still built CompletedProcess(stdout=""). That file carries a
  module-level skipif(sys.platform == "win32"), so the five tests covering the
  exact boundary I changed did not execute in my local run. The suite reported
  419 passed, 70 skipped and I read the first number; the skip count had risen
  from 64, which was the signal sitting in plain sight.

  Verified this time rather than reasoned about: I ran that file locally with the
  platform skip stripped, and all 10 TestRunGh tests pass on the fix
  (10 passed). Running the wider set of gh/glab-touching suites the same way
  surfaces 7 further failures, all of which are POSIX-only assertions reacting to
  a Windows host — SYSTEMROOT in the safe-env allowlist, and tests that assume
  no real gh is installed — not regressions; CI reported zero failures in those
  files, which is the check that separates the two.

  Coverage Gate was downstream of these five and needs no separate fix.

Local gates on ab966ccc8 (rebased onto 69b60c3b6): black baseline gate pass,
isort / flake8 pass on the full CI scope, mypy --platform linux src/kiro_crew/ → Success: no issues found in 1004 source files, 422 passed /
70 skipped / 0 failed, plus the skip-stripped run above.

[chenmingwei23]

GPT 5.6 disposition — 9815ba06e

• github_runner.py:236 — remote administrators are trusted as host administrators

  UNC-hosted CLI -> ACL walk trusts the remote server's Administrators SID -> remote administrator replaces the binary -> run_gh executes it unsandboxed. Anchor: residual/security. Fix: Reject Windows UNC paths before applying the local trusted-SID set.

  fixed — legitimate, and the argument lands against this module's own stated
  premise rather than against an edge case. The comment above the set says these
  SIDs are safe because "holding them already means owning the host". S-1-5-18
  and S-1-5-32-544 are machine-local alias SIDs: the same literal string on
  every machine, denoting a different principal on each. So the descriptor of a
  file on a remote share names the FILE SERVER's SYSTEM and Administrators, and
  the premise silently becomes "whoever administers that server may replace the
  binary this gateway executes". Reachable through the ambient PATH or
  KIROCREW_GH_BIN.

  No parity defence available on this one, unlike the earlier me_sid finding:
  the POSIX branch has no concept of a machine-local alias SID, so this is a
  defect in the mechanism this PR introduces, not a rule it inherited.

  Taken wider than the fix as written, because "reject UNC paths" misses the
  more common shape. A mapped network drive (Z:\gh.exe) has exactly the same
  exposure and is indistinguishable from a local disk by path inspection —
  os.path.splitdrive just answers Z:. In an enterprise that is the likelier
  way a provider CLI ends up on a share, so a UNC-only string check would have
  closed the half that is easier to spot and left the half that is easier to hit.

  GetDriveTypeW answers both shapes at once (it reports a UNC root and a mapped
  drive alike as DRIVE_REMOTE), so the component must now sit on a local volume
  before the trusted-SID set is consulted at all. Two details worth stating:

  • Local kinds are allowlisted, not remote ones denylisted — an unexpected or
    future drive type refuses instead of passing, matching how the rest of this
    policy already treats "don't know" (unreadable descriptor, NULL DACL,
    unparsable ACE, unverifiable SID, unreadable token all refuse).
  • The check runs before describe, and a test asserts that ordering by
    making describe fail the test if it is reached — so the refusal cannot be
    coming from somewhere else.

  Also pinned: every local drive type still reaches the ACL walk. Without that
  the guard could "pass" by refusing everything, which is the failure mode a
  fail-closed check invites.

• github_runner.py:221,299; platform_compat.py:3055 — function-local imports violate top-level-imports

  Fix: move these imports to module scope.

  fixed — checked the rule's three exemptions and none applies: no circular
  import (neither windows_acl nor platform_compat imports github_runner —
  the only mentions are in prose), not TYPE_CHECKING, not an optional
  dependency. So there was nothing to claim.

  The platform_compat.py pair turned out to be pure duplicates: that module
  already has import ctypes.util and from ctypes import wintypes at module
  scope, so the local copies were deleted rather than moved.

  Worth noting the rule earned its keep here. Its stated rationale includes "can
  make test mock patches target the wrong module namespace" — and with
  windows_acl now imported at module scope, runner.windows_acl is an
  attribute the new volume tests monkeypatch directly. Under the previous
  function-local import there was no such attribute to patch.

• CI: Backend Tests (3.10, 4) — 22 failures

  fixed, and the reason I missed them is worth recording because it is the
  second instance of one pattern. All 22 were in Code Review Sage's own suite
  (src/kiro_crew/apps/builtins/code_review_sage/tests/), whose test doubles for
  the run_gh spawn boundary returned str streams and, in two files, a
  SimpleNamespace with no args. Sage routes through run_gh, so last round's
  bytes change reached them.

  Last round I searched test/*.py for exactly this class of stub and reported it
  clean. That was true and useless: Sage's tests do not live in test/. The
  search scope excluded the evidence, which is the same failure as reading a pass
  count while the relevant tests were skipped. This round the sweep was the whole
  of src/ and test/, and the four helper factories are the only stubs on this
  boundary anywhere in the repo.

  Each helper now encodes str to bytes at the boundary, so all call sites keep
  their readable string literals.

Local gates on 9815ba06e: black baseline gate pass (17 files in scope — note the
gate reads committed diff only, so it must be run after the amend), isort /
flake8 pass on the full CI scope, mypy --platform linux src/kiro_crew/ →
Success: no issues found in 1004 source files, 883 passed / 350 skipped / 0
failed across the touched suites plus both app trees, and the skip-stripped sweep
surfaced the same 7 known POSIX-only-on-Windows failures as last round and nothing
new.

[chenmingwei23]

CI disposition — 16290a145

• Backend Tests (3.10, 4) and (3.12, 4) — 12 failures, AclUnavailable: Windows security descriptors are not available on this platform

  fixed — my regression from the previous push, and worth writing down because
  the fix is a design correction rather than a patch.

  The last round put the new volume check at the top of
  check_provider_path_component_windows as a direct GetDriveTypeW call. That
  call goes through windows_acl._load(), which raises off Windows. Every one of
  the 12 sibling policy tests stubs windows_acl.describe so the policy can be
  exercised on the Linux runner — and the new call ran before their stub was ever
  reached, so all 12 died at once on a line that had nothing to do with what they
  assert.

  That broke exactly the property this module was built around, and which Design
  Review named as the reason the shape is right: the read is platform-bound, the
  decision is not, and keeping them apart is what makes the Windows policy testable
  where CI actually runs. Adding a second platform-bound read to the decision side
  quietly undid it.

  So the fact now travels on the descriptor instead of being a second call:
  ComponentSecurity gained volume_is_local, describe() fills it in from
  GetDriveTypeW while it already holds the kernel32 handle, and the policy
  reads a bool. One read, one decision. The drive-type constants and the helper are
  private now, since nothing outside the read layer needs them.

  Net effect beyond fixing the shards: the policy is a pure function of one
  dataclass again, the 12 tests needed no change at all (their _security(...)
  factory gained one defaulted keyword), and the four volume tests stub describe
  like every other policy test rather than reaching for a second seam.

  Pinned so it cannot come back: a test replaces windows_acl._load with a
  pytest.fail and then runs the policy. _load is the single door to every
  platform-bound symbol in the module, so that one assertion covers the whole
  class — any future platform call added to the decision side fails immediately,
  on every platform, instead of only on the Linux shards.

  One thing I dropped deliberately: the previous revision asserted the volume was
  checked before the descriptor read. That ordering was over-constrained — reading
  a descriptor is not an execution, so nothing is at risk from reading it first.
  The property that matters is that a remote volume is refused, and that is what the
  tests now assert, including on an otherwise-perfect system-owned descriptor with
  only trusted writers (the exact shape that passes locally, and the reason the
  check is needed at all: the SIDs look right because they are the same strings,
  resolved against another machine).

  Verified on this host rather than argued: C:\ → drive type 3, local;
  \\nonexistent-server\share\ → 1, not local; the extended \\?\UNC\srv\sh\
  form → 1, not local; an absent Z:\ → 1, not local. resolve_gh() still
  returns C:\Program Files\GitHub CLI\gh.exe end to end with
  volume_is_local=True. Not verified: a reachable remote share, which would
  report drive type 4 — I have none to hand. That value is excluded by
  construction (the allowlist holds only removable, fixed, cdrom and ramdisk), and
  every unclassifiable answer already resolves to not-local, which is the direction
  that fails closed.

• Coverage Gate — downstream of the two shards; no separate action.

Local gates on 16290a145: black baseline gate pass (17 files in scope), isort /
flake8 pass on the full CI scope, mypy --platform linux src/kiro_crew/ →
Success: no issues found in 1004 source files, 880 passed / 70 skipped / 0 failed
across the touched suites plus both app trees, and the skip-stripped sweep shows the
same 7 known POSIX-only-on-Windows failures as the last two rounds and nothing new.