fix(design-critique): refuse a junctioned dir in the served signature

[leonlaiyc]

Problem / Motivation

_served_signature in the Design Critique backend digests the files
capture-build.mjs will actually serve, so /render can tell whether a cached
probe screenshot still depicts the current build output. That script's own header
calls the two enumerations a contract:

WHAT THIS ENUMERATES IS A CONTRACT with routes.py's _served_signature … the
Dirent test below counts neither a symlinked directory nor a symlinked file,
and dot-entries are skipped … Change both together.

The Python half held that contract with os.path.islink, which returns False
for a Windows directory junction. So the walk descended into a junction inside
the build dir and signed bytes the preview server never serves.

Measured on Windows, with the same junction on both sides:

	junction dist/vendor → an outside dir
Node readdirSync(withFileTypes) (the mjs predicate)	isDirectory=false isFile=false isSymbolicLink=true → not walked; served URLs = ["/app.js"]
os.path.islink (main)	False — so os.walk descended
os.path.isdir	True
platform_compat.is_link_or_junction	True

With main's guard, editing a file behind the junction — one the server has no
route to — moved the token's digest and raised newest_mtime_ns.

Why it matters

The signature's own docstring states the cost this direction: "signing something
the server never serves yields false mismatches and needless re-captures."
Concretely, on Windows:

• Needless re-captures. Unrelated churn behind a junction invalidates a valid
  cached screenshot for the whole _PROBE_REUSE_TTL_SEC window, and a re-capture
  is a full headless browser render.
• A false mid-capture staleness signal. newest_mtime_ns is the high-water
  mark the discover-time mid-capture check reads, deliberately including
  directory mtimes so a deletion cannot hide. An unserved tree could raise it and
  make a capture that is actually fine look stale.
• _SIGNATURE_MAX_FILES pressure. Files behind the junction increment seen,
  so a junction pointing at a large tree can push a small build over 20 000 and
  make _served_signature return None — which callers must read as "unknown"
  and refuse reuse entirely.

This is not a disclosure bug and the PR does not claim one. The token is a
digest and is never served; nothing behind the junction is read as content or
returned to a client. First Principles graded the same site "digest coverage, not
disclosure" when it named it on #8767, and that grading is unchanged here.

What changed (motivation → approach → change)

Symptom: the token moves for bytes the server will not serve, on Windows only.
Root cause: os.path.islink is the wrong predicate for the contract — the Node
side refuses a junction, the Python side admits it. Change: both guards in
_served_signature now go through platform_compat.is_link_or_junction, which
is already imported in this module and tries os.path.islink first, so symlink
behaviour is bit-for-bit unchanged and each guard is strictly widened.

Two guards, one contract:

• the dirs[:] prune — the load-bearing one, and the only site the measurement
  reproduces against;
• the files-loop check — behaviour-preserving on its own, because a junction
  is a directory reparse point and never lands in os.walk's files list. It is
  changed so both halves of one contract read through one predicate, rather than
  leaving a second spelling of the same guard behind. This is stated rather than
  implied: it is not a second fix.

capture-build.mjs needs no change — it was measured to already exclude the
junction, so the contract is restored by moving the Python side alone.

The docstring paragraph that asserted the two enumerations agree is corrected in
the same commit; it was the thing that was false.

Deliberately not changed: a junction in an ANCESTOR of build_dir (the
first_linked_ancestor case). _probe_build_dir already anchors build_dir
lexically under the validated project dir, and an ancestor link is a different
threat with a different helper. Out of scope, and not measured here.

Tests

test_served_signature_refuses_a_junctioned_directory (in the existing
design_critique/tests/test_backend_routes.py) pins:

• the digest does not move when a junction appears inside the build dir;
• the digest does not move when a file behind it changes;
• newest_mtime_ns is unchanged by that same edit — the field the mid-capture
  check reads, which the existing symlink test does not assert.

It carries a guard-the-guard assertion: on Windows the link must really be the
shape os.path.islink misreads (islink False, isdir True), so the test
cannot pass against a plain directory and prove nothing.

Red-before, measured on unpatched origin/main (routes.py restored in-tree,
patch copied aside):

>       assert linked is not None and linked.digest == sig.digest
E       AssertionError: assert (_ServedToken(digest='3a195d46...', ...) is not None
E         - 8916bb526b932080bb194fe24aaaaa1c
E         + 3a195d46bcdb32f2492880253cd717fd

Green after: 110 passed, 1 skipped across
src/kiro_crew/apps/builtins/design_critique/tests/.

Platform honesty. The one skip is the pre-existing
test_served_signature_ignores_what_the_server_will_not_serve, which needs
SeCreateSymbolicLinkPrivilege to make a directory symlink and skips on Windows
without it — that is, the existing test for this contract does not run on the
only platform that has junctions. The new test uses a junction on Windows (no
privilege required) and a symlink elsewhere, so on Linux CI it is a
no-regression check, not a red-before; only Windows exercises the fix.

Second commit — a type-check fix on this test file, no product change.
Backend Lint & Type Check (3.12) failed on the first head: typeshed guards
_winapi.CreateJunction behind sys.platform == "win32", so the direct
reference is an attr-defined error whenever mypy checks this in-package test on
Linux. The import is now a guarded module-level one — the shape the
top-level-imports rule exempts, which also closes the review's advisory finding
on the function-local import — and CreateJunction is resolved with getattr,
mirroring platform_compat's own getattr(os.path, "isjunction", None).

The Windows path still creates a real junction and nothing else. It is
deliberately NOT routed through a helper that can fall back to os.symlink:
symlink creation succeeds on a runner with Developer Mode enabled, and a symlink
is the shape os.path.islink already refused — so that fallback would turn the
Windows red-before green for the wrong reason. A missing CreateJunction now
asserts rather than silently degrading, and the red-before was re-verified
against unpatched origin/main with the new helper in place.

Gates on this head: flake8 clean, isort --check-only clean,
scripts/check_black_formatting.py PASS scoped origin/main...HEAD,
scripts/check_subprocess_encoding.py PASS. mypy --platform linux reports
nothing for design_critique/ — run with that flag deliberately, because mypy's
default platform on a Windows box resolves CreateJunction and hides the CI
error. Its remaining 4 findings are in apps/routes.py and
dashboard/state.py and are Python-3.10-only artifacts of the local
interpreter (asyncio.timeout, BaseException.add_note), not this diff.

The Build Desktop (ubuntu-22.04) red on the first head was not author-owned:
the frontend build, AppImage, .deb and .rpm all completed and the job failed
in actions/upload-artifact with ETIMEDOUT. No rerun was requested for it.

Manual verification

N/A — unit coverage sufficient. The behaviour is a pure function of a directory
tree, and the test builds the exact tree (junction included) that the defect
needs; the cross-language half of the contract was measured directly against
Node's Dirent predicate and is quoted above rather than assumed.

Related Issues

No issue. This site was named by First Principles on merged
#8767 and explicitly accepted-and-deferred there, then
re-measured against current main before this PR.

Pattern harvest

Rule candidate: review-prompt

Pattern: an os.path.islink guard that must refuse a junction — the guard is
correct on POSIX and silently open on Windows, where islink calls a junction a
plain directory.

Scoped deliberately as a review prompt, not a lint autofix or a sweep: #8767's
review cites #8165's ruling that these sites are decided one at a time on
harm, and the harm differs enormously between them (this one is digest
coverage; a sweep would lump it with deletion and write paths). The generalizable
part is "flag the site and ask what the guard protects", not "replace every
islink".

Second, narrower pattern, and the one that hid this: a contract asserted across
two languages, tested on only one of them. The existing test for this exact
invariant skips on the only platform where it can break.

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) — the corrected paragraph is the function's own docstring; no docs/ spec covers _served_signature
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] f0ce6af

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

CI does run Windows pytest shards (backend-test-windows in ci.yml), so the new junction test is exercised on the platform that has junctions — the "Manual verification: N/A" is backed by CI coverage plus the author's quoted Windows measurements.

My assessment: the problem is real (false digest mismatches on Windows → needless headless re-captures, false mid-capture staleness, _SIGNATURE_MAX_FILES pressure), the fix targets the root cause (wrong predicate for a cross-language contract), it uses the repo's canonical platform_compat.is_link_or_junction idiom (dozens of existing sites, including the sibling design_tweak app's identical walk at preview_files.py:273-280), the failure direction on error is safe (None → refuse reuse), scope is tightly bounded with the ancestor-junction case explicitly deferred, the docstring is corrected in the same commit, and the change is trivially reversible. No design-level findings survive.

Design-Verdict: PASS

Root-cause predicate fix restoring a measured cross-language contract, using the repo's canonical junction-aware helper, with the Windows branch exercised in CI.

[DESIGN-REVIEWED] d8a01a3

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] d8a01a3

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of d8a01a3d1b1c44d02b87bcdedf7ae0766e941011 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 checks confirm the change: base routes.py:356,369 are the only two os.path.islink sites in design_critique (both fixed); platform_compat is already imported at routes.py:44; is_link_or_junction is the repo's established junction-aware predicate (100+ call sites); the mjs indexBuildFiles Dirent walk does exclude anything that is neither isDirectory() nor isFile(), and the "Change both together" contract comment is real at capture-build.mjs:85-91. Python's islink-is-False-for-junctions premise is documented in this repo's own platform_compat.py:3750 and pinned_fs.py:429. Test helpers _build_tree/_bump and the skipping symlink test exist in the base test file.

First-Principles-Verdict: PASS

Only Windows exercises the junction path — confirm a Windows CI job actually runs the new test, since Linux CI sees only the symlink no-regression half.

What this change ships

Intent: stop a Windows directory junction inside the build dir from moving the served-signature token for bytes the preview server never serves — a FIX.

Inventory (4 items)

1. On Windows, edits behind a junction in the build dir no longer invalidate a cached probe screenshot, raise newest_mtime_ns, or count against the file cap — justified
2. The files-loop link check reads through the same junction-aware predicate (no behavior change today, declared as such) — justified
3. The signature docstring gains a paragraph stating the junction case it previously mis-asserted — justified
4. A new test pins the junction behavior (junction on Windows, symlink elsewhere), including the newest_mtime_ns field no existing test asserted — justified

The defect has provenance a reader can check: the "Change both together" contract in capture-build.mjs:85, Python's documented junction blindness (platform_compat.py:3750), and a red-before quoted against unpatched main. The fix sits at cause level — it replaces the wrong predicate with the repo's existing is_link_or_junction (grepped: 100+ consumers) rather than patching the observed site; both islink sites in this module are fixed (2/2), and the deferred ancestor-junction case is named with its reason. No new config, flag, or public surface ships.

[FIRST-PRINCIPLES-REVIEWED] d8a01a3

[bolichen97]

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

Clean rebase: no conflicts. Both commits replayed unchanged, and no behaviour was modified.

Gates run locally on the changed files only (backend/routes.py, tests/test_backend_routes.py): black, isort, flake8 all clean, and pytest src/kiro_crew/apps/builtins/design_critique/tests/test_backend_routes.py passed 97 tests.

Please review the rebased 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.