fix(dev-fleet): stop a junctioned node_modules wedging the sync

[leonlaiyc]

Problem / Motivation

The node_modules transaction removes a stashed tree with one rule, spelled out
in both copies of it:

rmtree REFUSES a symlink ("Cannot call rmtree on a symbolic link") and
ignore_errors=True swallows that refusal -- which once left a symlinked
tree's backup undeletable, so every later run saw both paths and refused as
ambiguous. A permanent wedge escapable only by hand.

os.path.islink reports False for a Windows directory junction, so a
junctioned tree skipped the unlink branch and went to rmtree — which refuses a
junction exactly as it refuses a symlink. Same swallowed refusal, same surviving
backup, same wedge.

Measured end-to-end on unpatched main (Windows, _winapi.CreateJunction, a
junctioned node_modules pointing at a shared store):

step	result
os.path.islink(tree) / os.path.isdir(tree)	False / True
begin()	True — the junction is renamed to the backup
commit()	the backup survives
store behind the junction	intact
next run begin()	False — logs "remove one by hand"

A junction is not an exotic layout here: it is the ordinary Windows spelling of
the shared-store node_modules the symlink branch was written for, because a
directory symlink on Windows needs SeCreateSymbolicLinkPrivilege and a junction
needs nothing.

Why it matters

Availability, not data loss — and the PR does not claim more. The store behind
the junction is never touched (rmtree refuses before it descends; verified in the
table above). What breaks is the flow:

• Dev Fleet Pull + Build stops on every subsequent press with
  EXIT_TREE_AMBIGUOUS, naming two paths and touching neither.
• The unattended auto-update path reaches the same refusal with no operator
  present. It never retries — the next boot sees the commit already applied — so
  its only self-heal is Dev Fleet's reconciliation, which is exactly what is now
  refusing.

Neither clears on its own. The transaction is deliberately built to refuse rather
than guess, so this state persists until a human deletes one of the two paths.

What changed (motivation → approach → change)

Symptom: a junctioned node_modules wedges the sync. Root cause: the link guard
uses a predicate that does not recognise a junction. Change: recognise it, and
remove it as a link rather than trying to walk it.

Two files, because the transaction has two copies by design and they share
semantics rather than code:

1. src/kiro_crew/node_modules_txn.py (the in-process half) uses
   platform_compat.is_link_or_junction and unlink_link_or_junction — the
   latter unlinks a symlink and rmdirs a junction's reparse point, never the
   target. Its only consumer, frontend.py, already imports platform_compat
   and already uses both helpers for the static/dist link, so this adds
   nothing to any import path.
2. dev_fleet/sync_runner.py (the snapshot half) cannot import
   kiro_crew: the module header states the stdlib-only rule is "an invariant,
   not a convenience", because the file is copied out and run by path while
   the repository is being merged underneath it, and a test asserts it. So the
   predicate is spelled out locally, with the reparse-tag fallback that
   os.path.isjunction (3.12+) does not provide on the 3.10/3.11 floor this
   project still supports — without it the guard would be a silent no-op on
   exactly the older Windows installs most likely to carry a junction. Confirmed
   locally: hasattr(os.path, "isjunction") is False on the 3.10 interpreter.

Fixing only one would leave the identical wedge on the other flow, so both move
together.

Not changed: the transaction's shape, its refusal policy, the ambiguous-state
branch, and the shared BACKUP_SUFFIX. The dev-fleet and cli specs describe the
transaction at that level and are unaffected, so neither needed a same-commit
update.

Tests

Three tests, in the files that already own these functions:

• test_node_modules_txn.py::test_a_junctioned_tree_does_not_wedge_the_next_run
  — drives begin() → fresh install → commit(), then asserts the backup is
  gone, the store behind the junction still holds its sentinel, and the next
  run's begin() returns True. That last assertion is the operator-visible
  consequence, not a proxy for it.
• test_dev_fleet_sync_runner.py::TestGone::test_a_linked_tree_is_unlinked_and_its_target_survives
  — gone() on the link reports True, the link is gone, the target survives.
• …::test_a_real_tree_is_still_removed — the new branch does not shadow the
  ordinary directory case.

Both link tests use conftest.make_dir_link, which yields a junction on
Windows and a symlink elsewhere, and both carry guard-the-guard assertions
(not os.path.islink / os.path.isdir on Windows) through an oracle outside
the module under test — so a red-before fails on behaviour, never on a missing
name.

Red-before, measured against unpatched origin/main with the two production
files restored in-tree:

E  AssertionError: the junctioned backup survived commit()
E  assert False is True        # gone(<junction>) reported not-removed

Green after: the three new tests pass; test_node_modules_txn.py +
test_dev_fleet_sync_runner.py + test_dev_fleet_app.py show 13 failed / 362
passed on test_dev_fleet_app.py both with and without the patch — an
identical control, so that pre-existing Windows set is not a regression. Those
failures are WinError 1314 (no symlink privilege in an unelevated shell), which
is precisely why the pre-existing test_a_symlinked_tree_is_still_protected
cannot run on Windows — the platform this defect lives on.

Platform honesty: on Linux CI make_dir_link produces a symlink, which the
original guard already handled, so these are no-regression checks there. Only
Windows exercises the fix.

Gates: flake8 clean, isort --check-only clean,
scripts/check_black_formatting.py and scripts/check_subprocess_encoding.py
both PASS scoped origin/main...HEAD (4 files). mypy --platform linux reports
nothing for either production file; its 4 findings are in dashboard/state.py
and apps/routes.py and are Python-3.10-only artifacts of the local interpreter,
absent on CI's 3.12.

Manual verification

N/A — unit coverage sufficient. The behaviour is a pure function of a directory
tree, and the tests build the exact tree (junction included) the defect needs,
including the second run that exposes the wedge.

Related Issues

No issue. Found by auditing link guards for the junction blind spot, then
measured against current main.

Pattern harvest

Rule candidate: review-prompt

Pattern: a link guard whose failure mode is a refused destructive call, not a
destructive one. The reflex on os.path.islink misses is "does something get
deleted through the link?" — here nothing does, and that is why it is a bug:
rmtree refuses, the refusal is swallowed by ignore_errors=True, and a caller
that reads the removal's success as a gate is left permanently stuck. A guard
miss whose consequence is availability is easy to grade as harmless and is not.

That grading matters at the seam level: a sibling rmtree-behind-islink site
(auto_improvement/pr_watchers.py:1417) was measured and rejected precisely
because nothing there reads the outcome — worst case a directory is not
reclaimed. Same predicate, same platform, opposite verdict. These sites are
decided one at a time on consequence, never swept.

Checklist

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

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 8121d07be96c699747f9430d9000ffdd30adf27c via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All premises verified: the platform_compat helpers and conftest.make_dir_link exist in the base tree, the stdlib-only invariant in sync_runner.py is real and documented (so the local predicate copy is the design's intended shape, not drift), CI runs a full Windows pytest shard job that will exercise the new junction tests (junctions need no privilege), and the description's red-before/green-after was measured on a real Windows host. The fix targets the root cause (a predicate blind to junctions) rather than the symptom, changes neither the transaction's shape nor its refusal policy, and the tests assert the operator-visible consequence (the next run's begin() succeeds). No design-level findings survived the kill filter.

Design-Verdict: PASS

Root-cause fix at the right seam: both deliberate copies move together, the predicate gap is closed, and Windows CI plus measured red-before evidence back it.

[DESIGN-REVIEWED] 8121d07

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 8121d07be96c699747f9430d9000ffdd30adf27c via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/builtins/dev_fleet/sync_runner.py:66 -- "project still floors at 3.10" contradicts the declared Python ≥3.12 requirement -> Fix: remove or correct the unsupported-runtime claim. (origin: validation)
[GPT-REVIEWED] 8121d07

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 8121d07be96c699747f9430d9000ffdd30adf27c via the fork AI-review pipeline — 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 claims verified against the base tree: platform_compat.is_link_or_junction/unlink_link_or_junction exist and frontend.py already consumes both (lines 26, 189, 209, 227); the stdlib-only rule is a real pinned invariant (test_sync_runner_imports_only_stdlib); the dev-fleet spec doesn't describe the guard at this level; and the only other rmtree-behind-islink site (pr_watchers.py:224) has no outcome-gating consumer, as the description itself measured. The one finding: the new 31-line local predicate has a smaller, predicate-free alternative grounded in an OS rule the repo's own code confirms.

First-Principles-Verdict: CONCERNS

The 31-line is_junction predicate in the snapshot-run script is deletable: os.rmdir-first already separates junctions and empty dirs from real trees by OS rule.

Not justified as shipped

3. sync_runner.is_junction + _IO_REPARSE_TAG_MOUNT_POINT — oversized: one consumer (gone()), and try: os.rmdir / except OSError: rmtree removes the same wedge with no predicate, no 3.12 probe, no reparse constant.

What this change ships

Intent: stop a junctioned node_modules from permanently wedging Pull+Build and unattended auto-update on Windows — a FIX.

1. In-process install transaction removes a junctioned tree as a link, so the backup clears and the next run proceeds — justified
2. Dev-fleet sync runner does the same in its stdlib-only copy — justified
3. New importable is_junction() helper local to the sync runner — oversized: smaller predicate-free shape exists
4. Three tests pinning junction removal, store survival, and the next run's begin() — justified

Watch

The junction branch exists only to route to rmdir; but rmdir succeeds only on junctions and empty directories (Win32/POSIX rule — the repo's own unlink_link_or_junction, platform_compat.py:3805, relies on it), so try: os.rmdir(path) / except OSError: shutil.rmtree(path, ignore_errors=True) is behaviour-identical and deletes the whole predicate from a file whose every line is copied out and run by path. The alternative was never weighed in the description.
Clears when: the predicate is replaced by the rmdir-first shape, or the author names a case where rmdir-first misbehaves.

Subtractions

Drop is_junction() and _IO_REPARSE_TAG_MOUNT_POINT from sync_runner.py (1 consumer: gone()); replace the elif is_junction(path): rmdir branch with try: os.rmdir(path) / except OSError: shutil.rmtree(path, ignore_errors=True) in the else arm.

[FIRST-PRINCIPLES-REVIEWED] 8121d07

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 8121d07be96c699747f9430d9000ffdd30adf27c via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 8121d07

[bolichen97]

Rebased onto main 2b060c18 by a maintainer as part of the 2026-09-08 open-PR audit.

Clean rebase, no conflicts. Your commit applied unchanged on top of current main; the diff is byte-identical to what you had (3e98e15e -> 8121d07b).

Gates run locally on the four changed files only: black --check, isort --check-only, flake8 all clean, and pytest test/test_dev_fleet_sync_runner.py test/test_node_modules_txn.py -> 50 passed.

Please review the new head. Note that a maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed before merge. Reply here if anything looks wrong.

[chenmingwei23]

🤖 Kiro Crew [operator: chenmingwei23#de330d0c]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: All blockers mechanical. (1) The branch is behind main (merge conflict) and needs a rebase. (2) Backend Lint & Type Check fails on the same code-style "history narration in comments" rule in test/test_node_modules_txn.py — reword the narrating comment. GPT's one finding is advisory (a docstring line "still floors at 3.10" contradicting the declared Python ≥3.12; a one-line correction), Design PASS, Opus none, First-Principles CONCERNS advisory.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.