Skip to content

fix(portability): keep the export archive inside the crew directory - #9070

Merged
iamwhatever merged 2 commits into
kirodotdev:mainfrom
leonlaiyc:fix/portability-export-junction-containment
Sep 7, 2026
Merged

fix(portability): keep the export archive inside the crew directory#9070
iamwhatever merged 2 commits into
kirodotdev:mainfrom
leonlaiyc:fix/portability-export-junction-containment

Conversation

@leonlaiyc

@leonlaiyc leonlaiyc commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

portability.create_export_zip walks workspace/, plan_memory/ and skills/ with
rglob("*") and writes each file into an archive the user downloads and hands on.
The walk skipped an entry that is a symlink — but rglob descends a
directory link, and the file on the far side is an ordinary one, so that skip never
fired for it. Measured: workspace/memory/linked/not-ours.md, living outside the
crew directory entirely, appeared in the export's namelist.

Correction to this PR's earlier description. It said _is_excluded and
is_sensitive_path "both read the LEXICAL path". That is wrong about the second one,
and a reviewer was right to flag it: is_sensitive_path_path_in_home_dirs
_candidate_forms builds _resolved_forms_bounded(expanded), so it does resolve
links — a linked ~/.aws really was caught. The accurate statement is narrower and
different in kind: neither filter is a containment test at all. _is_excluded is
a rule about the archive name; is_sensitive_path asks whether a path is a
protected location, never whether it is inside the crew directory. An ordinary
file of the user's — notes, documents, anything under a link somebody dropped in the
workspace — was neither, and went into the archive.

A resolve-and-compare is not enough, and the first commit here only did that.
ZipFile.write takes a name and opens it itself, so the file that was checked and
the file that is read are two separate lookups. Measured against the shape main
ships:

by-name containment check at validation time: True  (accepts it)
archived bytes: b'private notes'
VULNERABLE: True

And a refusal computed after the open is already too late on Windows — the
third finding. _open_inside called os.open(candidate, ...) and only then asked
fd_real_path where the descriptor had landed. Opening a path whose ancestor is a
link to a UNC share is itself an outbound SMB authentication as this process, so
the containment, sensitivity and hardlink checks were all correct and all ran one
syscall too late. This is the ordering hooks.validate_file_path states as its own
contract — the Windows UNC and linked-ancestor gates run before any resolution,
"because realpath on a UNC path is itself the outbound SMB probe".

And a pathname screen could not fix it — the fourth finding, which is what this
revision answers.
The previous head refused a candidate whose ancestor was a
reparse point, checked with lstat. A reviewer was right that this is still
raceable: any check of a name is a check-to-open window, and an adversary who can
plant the link chooses when. Measured, not conceded — the same racing writer,
run at the same instant, against both builds:

ARM A: previous head be3caf39b (lstat screen, then os.open by name)
  attacker      : rename SUCCEEDED, junction planted
  os.open called: keep.md                    <- the outbound probe
  bytes archived: None                       <- containment refused it, too late

ARM B: this revision (pinned ancestor walk)
  attacker      : rename REFUSED (PermissionError winerror=32)
  bytes archived: b'ours'

Arm A is the finding in three lines: the guard worked — nothing was archived — and
the probe went out anyway.

And the ENUMERATION was still probing before any of that ran — the fifth finding,
which this revision answers.
Pinning the candidate's ancestors closed the swap
race, but a junction already sitting in the workspace when the export starts was
never raced for: rglob descended it, and is_file() and is_sensitive_path()
resolved what it yielded, before _open_inside was ever reached. Measured by
wrapping every resolving call and exporting a workspace holding one planted junction:

head 5663dd30a (pinned read, rglob enumeration)
  resolving calls through the junction : 14   (os.path.realpath, ntpath._getfinalpathname)
  loot archived                        : False

this revision (pinned walk)
  resolving calls through the junction : 0
  loot archived                        : False

Both builds archive nothing from behind the junction, which is exactly why the
archive cannot be the oracle. If the junction names \\host\share, those 14
resolutions are the outbound SMB authentication, and return None afterwards
cannot recall them. The previous revision disclosed this as an unclosed residual; it
is closed here.

Why it matters

An export is a file the user deliberately hands to someone else — another machine, a
support thread, a backup. Content that was never theirs to share leaves the host with
no recovery once the archive has left.

The race is not theoretical here: a running gateway gives agent tools write access to
the same workspace the export walks, and an export can be triggered while they are
working. Under the race the harm is also strictly worse than under the plain blind
spot, because the sensitivity gate is one of the checks the swap gets to skip — the
name is validated, and something else is read.

The ordering finding is a different kind of harm rather than more of the same: a leaked
NTLM exchange is not something a later return None can take back, and it costs the
attacker nothing but a junction in a directory their tools can already write to.

What changed (motivation → approach → change)

Root cause: the check and the use are two lookups of one name, and no lookup was
asking about containment in the first place.

Approach: resolve once, open once, address everything downstream through the
descriptor — the discipline pinned_fs already exists for in this repo. This change
consumes its fd_real_path rather than adding a second mechanism.

  • _open_verified(target, root_real) — opens with O_NOFOLLOW where it
    exists, rejects a non-regular file on the descriptor's own fstat, then asks the
    kernel where the open descriptor actually is (/proc/self/fd, F_GETPATH, or
    GetFinalPathNameByHandleW) and compares that against the crew root.
    is_sensitive_path is re-asked about that real path, so the protected-location
    rule is not the one check the swap defeats. Every failure route — unreadable,
    non-regular, unknowable real path — returns None, never a fallback to the
    pathname.
  • _add_from_fd streams the entry from that descriptor instead of letting
    ZipFile.write reopen the name. Streaming rather than reading whole is deliberate:
    a workspace file has no size bound here and the export used to copy one of any
    size. This is also why hooks.safe_read_file_bytes_nolink — which performs exactly
    the right validation — was not reused: its 50 MB cap would silently drop a large
    workspace file from the export.
  • The entry's timestamp and mode come from the same fstat, so hand-building the
    ZipInfo does not quietly lose what write used to record.
  • The crew root is resolved once, before the walk (os.path.realpath(mc)), since
    $KIROCREW_HOME may itself legitimately be a link and the test must not reject the
    whole export because of it.
  • The by-name filters stay as cheap pre-filters that avoid opening what is already
    known to be unwanted, with a comment saying what each one actually decides.

Three things the descriptor alone does not settle, all raised in review and all fixed
here:

  • A hardlink defeats every path-based guard, including this one. It shares its
    target's inode, so there is no symlink for O_NOFOLLOW to refuse, is_symlink()
    is False, and fd_real_path reports the path the descriptor was opened by — the
    innocent workspace alias — not a canonical one. is_sensitive_path is then asked
    about the alias and answers "not sensitive" while the bytes behind it are a
    credential file's. Only the link COUNT, read off the descriptor, sees it, so
    st_nlink > 1 is refused. Same rule and same reasoning as
    pinned_fs.refuse_hardlink_alias and hooks.safe_read_file_bytes_nolink; the cost
    is honest and small — a workspace file that legitimately has a second link is left
    out of the export.
  • force_zip64=True on the streamed entry. ZipFile.write stat'd the source and
    turned ZIP64 on by itself; a streamed entry does not know its size when the header
    is written, so without this a source over zipfile.ZIP64_LIMIT raises
    RuntimeError part-way through and the export endpoint answers 500. A multi-GiB
    file under workspace/ is ordinary — a dataset, a model artifact — and it used to
    archive fine, so leaving this off would trade one defect for a regression.
  • The walk descends and verifies in one motion. _walk_contained /
    _walk_pinned replace rglob. A directory is PINNED with
    platform_compat.pin_directory before it is listed, and each child is then
    classified from the listing itself (_entry_is_link) rather than by a call that
    resolves it: on Windows FindFirstFileW returns the attributes and the reparse tag
    inline, so a child's type and link-ness cost no lookup THROUGH that child. A child
    directory is descended only by pinning it in turn, so the path the kernel walks to
    reach component n runs entirely through components already opened and verified —
    starting at the crew root itself, which the kernel walks to reach every candidate.
    pin_directory supplies both halves on Windows: its handle omits
    FILE_SHARE_DELETE, so a held directory can be neither renamed nor deleted — nor
    can anything above it — and its open refuses to follow a reparse point, so a
    junction sitting at the name fails there instead of being traversed. This is not
    a new mechanism
    : aws_control/backend/storage.py already pins root-then-child for
    exactly this reason — "a watcher can no longer rename the directory away and plant a
    junction at its name between our create and the CLI's open".
  • _pin_ancestors and _open_inside are deleted rather than kept beside it. The
    walk holds the same chain, for a whole subtree instead of re-walking it once per
    file, so keeping both would be two mechanisms doing one job. _entry_is_link
    refuses every reparse point rather than only link-shaped ones — which is precisely
    what pin_directory and open_file_no_reparse already refuse, so it classifies
    what the opens would reject anyway: a cheap skip, never the enforcement.
    DirEntry.is_symlink() alone would not do: a junction's tag is
    IO_REPARSE_TAG_MOUNT_POINT, not IO_REPARSE_TAG_SYMLINK, so that method answers
    False for one — which is how rglob came to descend it in the first place.
  • is_sensitive_path is no longer asked about the PATHNAME. That call was the
    probe — 12 of the 14 measured resolutions. It is removed, not relocated: the same
    question is still asked in _open_verified, of the descriptor's real path, which
    was always the load-bearing one because a name can be re-pointed and a descriptor
    cannot.
  • The leaf is opened and judged in one operation. New
    platform_compat.open_file_no_reparse opens with FILE_FLAG_OPEN_REPARSE_POINT, so
    a reparse point at the final name is opened as itself and refused (ELOOP) rather
    than followed — Windows has no O_NOFOLLOW, so os.open follows one. The attribute
    is read off the descriptor, which makes it a fact about what was opened rather than
    a prediction about what a later open will find. It shares the CreateFileW wiring
    with pin_directory via one private helper, so the two do not carry separate copies
    of the same security-critical flags. On POSIX it is the O_NOFOLLOW open the code
    already had, named.

Three deliberate boundaries, each stated because getting one wrong is worse than the
defect:

  • The walk starts at the RESOLVED root. pin_directory refuses a reparse point at
    a name, so pinning the configured spelling of $KIROCREW_HOME would fail on a host
    where the crew directory is itself a link — and return an empty archive rather
    than a safer one. A silently empty backup is worse than the leak. Resolving first
    means the chain that gets pinned is the real one; the crew root's own ancestors are
    configuration this export has already read through.
  • POSIX is not handed Windows semantics. The walk pins on both platforms, but
    only half the property is real there: pin_directory on POSIX is
    O_RDONLY | O_DIRECTORY | O_NOFOLLOW, whose refusal of a symlinked directory is
    genuine and is exactly what rglob already did — so the exported set is unchanged —
    while the anti-rename half has no POSIX equivalent and nothing here claims it. The
    Windows-only tests say so in their skip reason. Containment on POSIX rests where it
    always did, on _open_verified checking the descriptor's real path; resolving a
    symlink there is local and leaks nothing, so there is no outbound probe to prevent.
  • No lexical UNC screen. A candidate here is the configured root joined with
    components the export itself enumerated, so it is UNC-shaped only when the root is —
    and refusing on that would break a deliberately configured UNC home for nothing.

What it costs, stated exactly. Nothing outside the crew directory is newly
dropped — containment already refused that. Content still inside it is either walked
under its own name anyway, or, if it lives in a part of the crew directory the export
does not walk, is left out on Windows — which is already the behaviour on POSIX, where
rglob never descends a directory link. The walk holds one descriptor per directory
for as long as that directory's subtree is being produced — depth, not breadth, bounds
how many are open at once, and it is strictly fewer opens than the previous revision,
which re-pinned the whole ancestor chain once per file. Each is released in a
finally that also runs if the consumer abandons the walk; a test pins that they are
released even when a component mid-chain refuses, because a handle leak here would
exhaust a large export rather than merely look untidy.

The exported set is unchanged by the walk rewrite, measured rather than argued:
on a tree covering nesting, EXCLUDE_DIRS, EXPORT_EXCLUDE, .pid and
skills/auto, the archive namelist and manifest counts were byte-identical to the
ones the previous walk produced. The exclusion fix below then deliberately CHANGES
that set on Windows — see the next section.

The Windows exclusion bypass — disclosed here first, then fixed here. An earlier
revision of this description scoped this out as a separate defect. A security review
disagreed and was right, so it ships in this PR.

_keep_for_export asked _is_excluded(PurePosixPath(str(rel))). On Windows
str(rel) is backslash-separated, so PurePosixPath parses the whole relative path
as a SINGLE component: .name becomes workspace otes\.env and .parts has
length one, so the EXPORT_EXCLUDE basename set and the EXCLUDE_DIRS walk both
stop matching. workspace/notes/.env — a credential file this export exists to keep
out — went into the archive the user downloads and hands on.

Fixed with PurePosixPath(*rel.parts) for the filter and rel.as_posix() for the
archive name. Measured on the same fixture tree as the parity run above:

before: workspace/notes/.env, workspace/__pycache__/x.pyc, workspace/snapshots/s.md
        all EXPORTED   (7 files)
after : excluded        (4 files)

So the exported set does change on Windows, and that is the fix: every nested
.env, .local_secret, sel_hmac.key, telemetry_salt, *.pid, and everything
under snapshots/, outbox/, uploads/ and __pycache__/ now stays out. POSIX is
unaffected — str(rel) and parts already agreed there.

On the archive name: ZipInfo rewrites os.sep today, so that half fixes no live
bug; it stops the member name depending on that behaviour, next to a filter that must
not.

Tests

TestTheAncestorSwapIsRefusedNotDetected (new — replaces the screen's tests):

  • test_a_racing_writer_cannot_swap_a_pinned_ancestorthe blocking finding's own
    scenario, executed rather than described.
    The swap is performed from inside
    pin_directory, immediately after the targeted component has been verified and
    pinned and before the next filesystem call — the precise instant a real racing
    writer would aim for, rather than a thread that has to get lucky. Two assertions,
    both load-bearing: the rename must fail, because that is the property (the
    export no longer depends on nobody having swapped the directory, but on nobody being
    able to); and the descriptor must still come back on the genuine bytes, because a
    guard that closed the race by refusing everything would pass the first assertion and
    be useless.
  • test_the_same_rename_succeeds_once_nothing_is_pinned — guards that guard: the
    refusal above is the pin, not a filesystem that could never have done the rename.
  • test_a_reparse_point_at_the_leaf_is_refused_without_being_followed
    open_file_no_reparse refuses the link, and the guard-the-guard then proves that
    exact name
    is traversable by an ordinary os.open, so the refusal cannot be
    mistaken for the link being unopenable.
  • test_an_ordinary_file_still_opens_through_the_no_follow_open — the descriptor is
    the one _add_from_fd streams from, and on Windows it is now a CreateFileW handle
    wrapped in a CRT descriptor. The test exercises st_nlink (the hardlink rule reads
    it off this handle), lseek and dup+fdopen — exactly the operations a shallow
    test would miss and the archive would break on.
  • test_every_pin_is_released_even_when_a_component_refuses — a component mid-chain
    is made to refuse and every pinned descriptor must still be closed.

TestNothingIsProbedThroughAPlantedReparsePoint (new — the fifth finding):

  • test_a_pre_planted_junction_is_never_resolved_during_an_exportthe assertion
    is on the CALLS, not on the archive.
    A junction is planted under workspace/
    before the export runs; every resolving call (os.stat, os.open, os.listdir,
    os.scandir, os.path.realpath / isfile / isdir / exists / getsize, and
    ntpath._getfinalpathname) is wrapped, and the test fails if any of them names the
    junction or anything beneath it. os.lstat and os.path.islink are deliberately
    not in that set — they do not follow the final component, so naming the link with
    one traverses nothing. An audit hook covers os.scandir / os.listdir as well,
    because pathlib binds scandir at import time and patching os.scandir does not
    see rglob's own descent — the single most important call to catch. Two negative
    controls sit beside it: the genuine workspace/keep.md must still be archived (a
    guard that refused the whole tree would otherwise pass), and the loot must still be
    absent.
  • test_the_recorder_actually_catches_a_probe — guards that guard. It performs the
    three calls the previous build made on that exact path (is_file(),
    realpath(), rglob) and asserts the recorder saw both a realpath and the
    scandir descent. An oracle that can never fire proves nothing, and this one is
    the whole test.
  • Guard-the-guard on the fixture itself, through oracles outside the module:
    is_symlink() must be False (it really is a junction, which is why rglob
    walked in), is_link_or_junction must be True, and the far side must really be
    readable through the link.

Red-before for these two: with only portability.py reverted to 5663dd30a and
the new tests kept, test_a_pre_planted_junction_is_never_resolved_during_an_export
fails with AssertionError: the export resolved a path through the planted junction (14 calls), listing os.path.realpath and ntpath._getfinalpathname on the junction
path. The recorder test passes on both builds, as it must — it tests the oracle, not
the fix.

The tests that depend on the rename lock are marked Windows-only with the reason
stated: the walk pins on both platforms, but the anti-rename half is a share-mode
property (FILE_SHARE_DELETE omitted) that POSIX has no equivalent of.

TestALinkedCrewRootStillExports:

  • test_a_crew_root_reached_through_a_link_still_packages_its_workspace — drives the
    public create_export_zip with $KIROCREW_HOME on a real link. This is the
    regression the resolved-root rule exists to avoid, and it would otherwise be silent.

TestTheArchivedBytesAreTheValidatedBytes (same assertions, retargeted from
_open_inside to _open_verified now that the pins live in the walk):
test_retargeting_the_link_after_validation_cannot_change_what_is_archived (the
descriptor race, with the swap at the crew root — the one component outside the pins),
test_a_sensitive_target_is_refused_on_the_descriptor,
test_a_file_reached_through_a_directory_link_gets_no_descriptor,
test_a_real_file_inside_the_crew_dir_gets_a_descriptor_on_its_own_bytes,
test_a_hardlinked_alias_gets_no_descriptor (+ its single-link control),
test_a_streamed_entry_larger_than_the_zip64_limit_still_exports (+
test_the_zip64_guard_can_actually_fail),
test_containment_fails_closed_when_the_real_path_is_unknowable,
test_a_directory_never_yields_a_descriptor.

TestExport: test_export_does_not_package_files_reached_through_a_directory_link
(Windows-marked, with its guard-the-guard walking from the root the production walk
starts at), test_export_preserves_the_mtime_of_what_it_packages,
test_export_still_packages_a_real_nested_workspace_file.

Red-before is the A/B in Problem / Motivation, run against be3caf39b loaded
side-by-side with this build and driven by the identical attacker: on the previous
head the racing rename succeeded, the junction was planted, and os.open was still
called on the candidate; on this head the rename is refused with WinError 32 and the
genuine bytes come back. The same swap, injected through the shipped test, is
test_a_racing_writer_cannot_swap_a_pinned_ancestor.

Gates. test/test_portability.py 78 passed / 4 skipped on Windows against real
junctions. pin_directory's existing consumers re-run clean:
test_platform_compat.py, test_aws_control_storage.py, test_pinned_staging.py =
383 passed / 72 skipped, the single failure being
TestFindPythonInterpreterReal::test_version_gate_ignores_a_sitecustomize_decoy_on_pythonpath,
reproduced with this branch's production files reverted to be3caf39b — it is an
artefact of the PYTHONPATH override this Windows box needs to run the suite at all,
which is the very thing that test is about. mypy --platform linux reports 0 errors
in portability.py (the 4 it finds are pre-existing in dashboard/state.py and one
other, reached by import following). black --check clean on portability.py;
test/test_portability.py is in the repository's black baseline (1164 known-
unformatted files) and scripts/check_black_formatting.py passes, so it is
deliberately not reformatted — doing so would bury this diff under an unrelated
rewrite of its SQL fixtures. flake8 and isort --check-only clean on both;
scripts/scrub-lint.sh --no-history passes.

Manual verification

N/A — unit coverage sufficient: the whole defect is the relationship between the
containment decision and the archive read, and the tests drive both against a real
junction rather than a mock, including through the public create_export_zip. The
ordering property is asserted on the syscall itself rather than on an observable
side effect, which is what a manual run could not have shown either.

Related Issues

None — found by inspection while auditing is_symlink-based containment guards for
the Windows junction blind spot, the same family as #7881.

Pattern harvest

Rule candidate: review-prompt
Pattern: a containment guard that validates a path and then reopens it. Two
distinct failures ride together — a recursive walk descends a directory link and the
leaf beyond it carries no mark (a Windows junction carries none anywhere), and
resolve()-then-open(name) is a check-to-use window. Where a walk feeds something
that reads, writes, moves or executes the result, the fix is not a stricter name
check: open once and address the descriptor. A second lesson for review prompts:
"filter X already resolves links" does not mean filter X answers containment — ask
which question each filter actually answers before crediting it with an unrelated one.

Third lesson, and the one this PR was blocked twice to learn: on Windows a correct
refusal computed after the open is still a leak, and no pathname check can fix it.

Touching an untrusted path is the outbound probe, so the sequence must be
open-and-judge in one operation, with the ancestors held rather than inspected —
lstat-then-open and is_symlink()-then-open are the same defect wearing different
clothes. The repository already had the primitive (platform_compat.pin_directory)
and already had the pattern (aws_control/backend/storage.py); the review prompt
worth writing is "if this guard checks a name and then opens that name, what holds the
name still in between?".

Fourth: when reusing an ancestor-walking guard, ask what it is scoped to. A walk
that runs to the drive root fails closed on everything when the configured root is
itself a link — which would have turned a leak fix into a silently empty backup.

Fifth, and the one this revision cost: guarding the read does not guard the walk.
Four revisions hardened what the export was allowed to OPEN while the enumeration in
front of it kept resolving untrusted names for free. The review prompt worth writing
is "before the guard runs, what has already touched this path?" — and the test that
catches it must assert on the calls, because every build here archived the right
thing. rglob, glob, os.walk and Path.iterdir are all guard-free surface. The
enumeration primitive that is safe was already available: os.scandir hands back a
child's type and reparse tag from the directory listing, so a walk can decide about a
child without ever addressing it.

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)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

`create_export_zip` walks `workspace`, `plan_memory` and `skills` with
`rglob("*")` and skips an entry that `is_symlink()`, so that an export never
carries bytes from outside the crew directory. That skip is blind to the escape
that costs the most: `rglob` DESCENDS a directory link, and the file on the far
side is an ordinary one — `is_symlink()` false — so it is written straight into
the archive. On Windows the link is typically a junction, which `is_symlink`
does not report at all, and a junction needs none of the privilege a directory
symlink needs there.

The two filters below the skip do not catch it either. `_is_excluded` and
`is_sensitive_path` both read the LEXICAL path, which runs through the link's
own name and therefore looks like ordinary crew content — so a sensitive file
behind a link passes the sensitivity check that exists to stop exactly this.

Measured on unpatched main with a real junction: `workspace/memory/linked/
not-ours.md` appeared in the export's namelist, sourced from outside the crew
directory. The consequence class is content the user then hands to someone else,
since an export archive is made to be moved off the host.

Fixed by requiring the RESOLVED path to stay under the resolved crew directory —
the repo's existing idiom (`apps/backend.py:651`). Resolving is what covers both
shapes at once: it follows every reparse point on the way down and answers where
the bytes actually live. `mc` is resolved once so a legitimately linked
`$KIROCREW_HOME` does not reject the whole export. The `is_symlink()` skip is
kept as-is; this adds a layer rather than replacing one.

The import side of this module is untouched.

`test_export_skips_symlinks` cannot cover this — it plants a link that IS the
entry, and it `pytest.skip`s where symlinks cannot be created, i.e. on the
platform where the junction spelling lives. The new tests use
`conftest.make_dir_link` and need no privilege.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@leonlaiyc
leonlaiyc requested a review from a team as a code owner September 6, 2026 16:53
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5.1, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fix on a real boundary (workspace-writable-by-agent + Windows SMB probe), reusing existing pinned-fd primitives — but the description contradicts the shipped diff on a security-relevant hunk.

Watch

  • The "still out of scope" claim is false, and it hides a second security fix. The PR body states the Windows separator bug — _is_excluded(PurePosixPath(str(rel))) collapsing a nested path to one component, so workspace/notes/.env exports on Windows — is "untouched by this PR." The diff does fix it: the old call site (portability.py:168) is replaced by _keep_for_export using PurePosixPath(*rel.parts), plus a whole new TestExclusionsSurviveAWindowsSeparator. A reviewer trusting the description would not vet the new credential-exclusion logic. Consequence: the fold-in is fine and welcome, but the "byte-identical export set" parity claim cannot hold on Windows (a credential file that used to export no longer does) — reconcile the description with what shipped so the parity assertion isn't relied on as-is.

Suggestions

  • platform-compat.md's helper table documents pin_directory as the "hold a dir in place" primitive; add a row for the new sibling open_file_no_reparse so the leaf-open counterpart is discoverable next to it.

[DESIGN-REVIEWED] 82700d5

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 82700d5

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/platform_compat.py:3853 -- "Refuses a directory" contradicts the POSIX branch, which returns a directory descriptor -> Fix: document that POSIX callers must reject directories.
[GPT-REVIEWED] 82700d5

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of 82700d56276fe2c6ff6820e53cee09ba0dc9b606 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.

First-Principles-Verdict: CONCERNS

Every item traces to a named harm on a real boundary (export leak, agent-tool write race, Windows UNC auth probe) — but a second defect fix and a one-caller keep parameter ride along.

What this change ships

Intent: stop create_export_zip from packaging files that live outside the crew directory. FIX.

  1. Files reached through a directory junction/symlink are no longer exported — justified (measured leak: workspace/memory/linked/not-ours.md).
  2. A hardlinked file (st_nlink>1) is now dropped from the export — justified (credential-alias leak; matches pinned_fs.refuse_hardlink_alias).
  3. A concurrent directory swap mid-walk is refused, not just detected (Windows pins) — justified (running-gateway write race is a named boundary).
  4. The walk never resolves through a planted junction (no outbound SMB probe) — justified (Windows UNC auth = platform fact).
  5. Excluded basenames (.env, …) now excluded for nested Windows paths — rides along; separate pre-existing defect (old PurePosixPath(str(rel)) at portability.py:168), justified and tested.
  6. New platform_compat.open_file_no_reparse + _win_open_without_following extraction — 1 consumer, justified leaf twin of pin_directory.
  7. Entries hand-built/streamed from fd with force_zip64, mtime/mode from fstat — justified (avoids large-file 500 regression).
  8. Crew root resolved once before the walk — justified (linked $KIROCREW_HOME).

Watch

  • Item 5 is a distinct Windows credential-leak fix riding in a link-containment PR; its zero option DOES cost Windows users, so it belongs, but a human should note two defects land together.

Subtractions

  • Drop the keep: Callable parameter from _walk_contained/_walk_pinned and inline _keep_for_export: exactly one production caller passes it (create_export_zip, patch line 418); the lambda _rel: True callers are tests, which don't count. Take the concrete filter.

[FIRST-PRINCIPLES-REVIEWED] 82700d5

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

Addressing the exact-head [BLOCK-MERGE] on 78232262f, and the finding beside it, at 3325f5fca.

F1 — TOCTOU at portability.py:182. Accepted as stated. zf.write(str(fpath), ...) takes a name and opens it again, so the check and the read were two lookups. Validation now happens on an open descriptor (_open_insidepinned_fs.fd_real_path) and _add_from_fd streams the entry from that descriptor. is_sensitive_path is re-asked about the descriptor's real path, so the protected-location rule is not the one check the swap gets to skip.

Reproduced against the shape main ships before fixing it:

by-name containment check at validation time: True  (accepts it)
archived bytes: b'private notes'   VULNERABLE: True     <- zf.write reopens the name
archived bytes: b'ours'            VULNERABLE: False    <- streamed from the descriptor

Streaming rather than reading whole is deliberate — a workspace file has no size bound here. That is also why hooks.safe_read_file_bytes_nolink, which performs exactly the right validation, was not reused: its 50 MB cap would silently drop a large workspace file from the export. The entry's timestamp and mode come from the same fstat, so the hand-built ZipInfo does not lose what write recorded (test_export_preserves_the_mtime_of_what_it_packages).

FINDING — the is_sensitive_path claim. You are right and the description was wrong. _path_in_home_dirs_candidate_forms builds _resolved_forms_bounded(expanded), so it does resolve links; a linked ~/.aws really was caught. Both changed comments and the PR body are corrected. The accurate statement is narrower and different in kind: neither filter is a containment test — _is_excluded is a rule about the archive name, and is_sensitive_path asks whether a path is a protected location, never whether it is inside the crew directory. An ordinary file of the user's was neither, which is what leaked.

One more thing that was green for the wrong reason. The walk-descent test guarded itself with link.rglob("*") — a walk starting at the link, which descends on every platform — so on Linux it proved nothing about whether the export's own walk from workspace/ ever gets there. It now guards from the production walk's root, and is marked for Windows: pathlib's ** does not descend a POSIX directory symlink. Measured both ways.

@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from 3325f5f to 8a74e97 Compare September 7, 2026 06:15
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

Both exact-head [BLOCK-MERGE] findings on 3325f5fca are accepted and fixed at 8a74e97ad. Neither is argued down — both are the reviewer's own prescribed fix.

F1 — hardlinked alias. Correct, and it is the sharper version of the point I only half-made. fd_real_path returns the path the descriptor was opened by, so for a hardlink it reports the innocent workspace alias and is_sensitive_path is asked about the wrong name entirely. There is no symlink for O_NOFOLLOW to refuse and is_symlink() is False, so only the link count sees it. _open_inside now refuses st_nlink > 1, matching pinned_fs.refuse_hardlink_alias and hooks.safe_read_file_bytes_nolink rather than inventing a rule.

F2 — ZIP64. Confirmed against CPython's zipfile before fixing it: a streamed entry has no size at header time, and crossing the limit raises RuntimeError: File size unexpectedly exceeded ZIP64 limit from zipfile.py. force_zip64=True fixes it, which restores what ZipFile.write was doing implicitly. This was a regression my own streaming change introduced, not a pre-existing gap.

Red-before, measured against 3325f5fca with only the production file reverted and the new tests kept:

FAILED ...::test_a_hardlinked_alias_gets_no_descriptor
  AssertionError: assert 14 is None          <- the alias was handed a descriptor
FAILED ...::test_a_streamed_entry_larger_than_the_zip64_limit_still_exports
  RuntimeError: File size unexpectedly exceeded ZIP64 limit   (zipfile.py:1168)

Both pass on this head; test/test_portability.py is 70 passed / 4 skipped locally.

The hardlink test guards itself through four oracles outside the module — the alias is not a symlink, its realpath is itself, is_sensitive_path accepts it, and it really does read back the target's bytes — so it fails for the reason it names and not an unrelated one. The ZIP64 test lowers zipfile.ZIP64_LIMIT instead of inflating the fixture (the branch is selected by size > ZIP64_LIMIT, and a multi-gigabyte artifact would buy nothing but minutes), and test_the_zip64_guard_can_actually_fail proves the lowered limit really selects that branch so the test cannot pass vacuously.

Design and First Principles also name sibling check-to-open sites elsewhere in this module. They are advisory and I have deliberately left them — widening this PR to silence advisory comments is how a bounded security fix stops being reviewable. Happy to take them as a follow-up.

Amended rather than stacked, to stay inside the two-commit rule; the PR body is re-synced to what actually ships.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from 8a74e97 to 4f5e14d Compare September 7, 2026 06:19
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 7, 2026
@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from 4f5e14d to be3caf3 Compare September 7, 2026 07:25
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

The exact-head security [BLOCK-MERGE] on 4f5e14d17 is accepted and fixed at be3caf39b. Not argued down: the causal path was re-verified against that head before anything was changed, and it holds.

The finding, confirmed. _open_inside called os.open(candidate, ...) and only then asked fd_real_path where the descriptor had landed. Every check after that open was correct and every one of them ran a syscall too late: opening a path whose ancestor is a link to a UNC share is the outbound SMB authentication, and return None cannot recall it. The prefilters do not help — an ancestor swapped after them redirects the lookup, which is the same gateway-writable-workspace window this PR's descriptor pinning already assumes. hooks.validate_file_path states the ordering as its own contract ("the Windows UNC trusted-root gate ... BEFORE any resolution -- realpath on a UNC path is itself the outbound SMB probe"), so this was the repository's stated policy being violated, not a judgement call.

The fix, and where it deviates from the prescription. The suggested remedy was "open through a hooks.py streaming descriptor helper that applies validate_file_path before filesystem access". I applied the policy but not that route, for two measured reasons:

  1. There is no streaming helper in hooks.py. safe_read_file_bytes, ..._with_identity and ..._nolink all read whole and cap at MAX_FILE_BYTES = 50 MB. Routing the export through one would silently drop any workspace file over 50 MB — a dataset, a model artifact — which is the same regression the force_zip64=True work in this PR exists to avoid. That trade was already rejected once here for safe_read_file_bytes_nolink.
  2. validate_file_path cannot be applied to this input as-is. Its linked-ancestor gate is first_linked_ancestor(target), which walks every ancestor to the drive root. $KIROCREW_HOME — or any directory above it — may legitimately be a link, and the export resolves the root precisely because of that. Calling it here would refuse every candidate on such a host and return an empty archive: a silently empty backup is a worse outcome than the leak. Its _MAX_SCREENED_PATH_DEPTH = 255 and expanduser semantics are likewise shaped for a user-supplied path, which these are not.

So the same policy is applied through the same primitives, scoped to this input: _linked_component_below(root, candidate) walks the components the export's own walk discovered underneath the configured root, root-first and leaf-inclusive, using platform_compat.is_link_or_junction — the identical helper first_linked_ancestor uses, in the identical order, so each lstat runs only after everything above it is known not to be a link and the screen never traverses one itself. _open_inside refuses before os.open on os.name == "nt", which is how acp.prompt_blocks, dashboard/handlers/themes.py and messaging/outbound_files.py each gate ahead of their first filesystem call. The leaf is included because O_NOFOLLOW does not exist on Windows. The lexical UNC screen is not repeated: a candidate here is the configured root joined with enumerated components, so it is UNC-shaped only when the root is.

Cost, stated exactly. Nothing outside the crew directory is newly dropped — containment already refused that. Content still inside it is either walked under its own name anyway, or, if it lives in a part of the crew directory the export does not walk, is now left out on Windows — which is already the behaviour on POSIX, where rglob never descends a directory link. The change converges the platforms rather than adding a rule to one.

Red-before, on the property rather than a proxy. test_a_linked_component_below_the_root_is_refused_without_opening spies on os.open while still performing it and asserts it is never called for a candidate behind a real directory link. Measured on a tree differing from 4f5e14d17 only by the inert root parameter — screen call removed, helper and tests kept — so the failure is the property, not a missing symbol:

FAILED test_a_linked_component_below_the_root_is_refused_without_opening
E  AssertionError: the candidate was opened before the screen refused it:
   ['...\kirocrew-home\workspace\linked\not-ours.md']
E  assert ['C:\Users\...\not-ours.md'] == []

The five surrounding tests passed unchanged in that same run — they are controls, and a control going red would mean the screen was being credited with something it does not do. os.name is forced to "nt" rather than the tests being skipped off Windows, so a POSIX-only CI run still carries evidence; the link is real on both platforms and only the gate is simulated. test_an_ordinary_candidate_is_still_opened guards the spy, and test_a_crew_root_reached_through_a_link_still_packages_its_workspace drives the public create_export_zip with $KIROCREW_HOME on a real link so the empty-export regression above cannot land silently.

75 passed, 4 skipped in test/test_portability.py on Windows against real junctions; mypy --platform linux src/kiro_crew/portability.py clean.

What this does not close, so it is not read as more than it is. The screen covers the read. It does not cover the enumeration: rglob descends a junction in order to yield what is beyond it, and is_sensitive_path/is_file() resolve by name before _open_inside is reached, so a junction already planted when the export starts is still traversed by the walk itself. That is main's behaviour and this PR does not change it — closing it means replacing rglob with a prunable walk, a different change with its own regression surface. What is closed here is the read and the post-walk swap window, which is the path the finding named.

The descriptor-pinning regression test moved its swap site to the crew root itself, since the new screen makes a link below the root unreachable on Windows; the root is the one component deliberately outside the screen and therefore exactly where a swap can still land, which keeps that property covered on both platforms instead of on POSIX alone. The PR description is re-synced to what actually ships.

🤖 Generated with Claude Code

@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from be3caf3 to 5663dd3 Compare September 7, 2026 08:21
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

The exact-head security [BLOCK-MERGE] on be3caf39b is accepted and fixed at 5663dd30a. The prescribed fix is what shipped: pin each verified ancestor before inspecting the next, then open and reject the leaf reparse point atomically. No further pathname check was added — the previous one was removed.

The finding was right, and here is the measurement rather than a concession. Same racing writer, same instant (the swap fires from inside the walk immediately after the targeted component is verified and before the next filesystem call), run against both builds:

ARM A: be3caf39b (lstat screen, then os.open by name)
  attacker      : rename SUCCEEDED, junction planted
  os.open called: keep.md                    <- the outbound probe
  bytes archived: None                       <- containment refused it, too late

ARM B: 5663dd30a (pinned ancestor walk)
  attacker      : rename REFUSED (PermissionError winerror=32)
  bytes archived: b'ours'

Arm A is the whole finding in three lines: the guard worked — nothing was archived — and the probe went out anyway.

The primitive already existed; this reuses it rather than inventing one. platform_compat.pin_directory opens with FILE_FLAG_OPEN_REPARSE_POINT and a share mode that omits FILE_SHARE_DELETE. Measured on Windows 11 before writing any code:

Probe Result
rename / rmdir a pinned directory refused, WinError 32
rename an ancestor of a pinned directory refused, WinError 5
the same rename once the handle is released succeeds — so the refusal is the pin
pin_directory on a junction NotADirectoryError, at the open
CreateFileW(OPEN_REPARSE_POINT) on a junction handle identity is the reparse point itself, not the target — it did not traverse
plain os.open through that junction succeeds — the probe being removed

_pin_ancestors therefore opens and holds every directory from the resolved root down to the candidate's parent, pinning each before naming the next, so the only path the kernel walks to reach component n runs through components already opened and verified. This is the pattern aws_control/backend/storage.py already uses to hold the path a sandboxed CLI writes through — "a watcher can no longer rename the directory away and plant a junction at its name between our create and the CLI's open".

The leaf. Windows has no O_NOFOLLOW, so os.open follows a reparse point at the final name and no by-name check can close that. platform_compat.open_file_no_reparse is new and small: it opens with FILE_FLAG_OPEN_REPARSE_POINT and reads the attribute off the descriptor, so the refusal and the open are one operation — a fact about what was opened, not a prediction about what a later open will find. It shares the CreateFileW wiring with pin_directory through one private helper, so the two do not carry separate copies of the same security-critical flags. On POSIX it is exactly the O_NOFOLLOW open the code already had, given a name.

Three boundaries, stated because getting one wrong is worse than the defect. The walk starts at the resolved root — pin_directory refuses a reparse point at a name, so pinning the configured spelling of $KIROCREW_HOME would return an empty archive on a host where the crew directory is a link, and a silently empty backup is worse than the leak. It is Windows-only, because a POSIX pin cannot block a rename and there is no outbound probe there to prevent. And no lexical UNC screen is added: a candidate here is the configured root joined with components the export itself enumerated, so it is UNC-shaped only when the root is.

Coverage is on the property, not the mechanism. test_a_racing_writer_cannot_swap_a_pinned_ancestor performs the swap from inside pin_directory at that exact instant and asserts both that the rename failed and that the genuine bytes still came back — a guard that closed the race by refusing everything would pass the first assertion and be useless. test_the_same_rename_succeeds_once_nothing_is_pinned proves the refusal is the pin. test_a_reparse_point_at_the_leaf_is_refused_without_being_followed refuses the link and then proves that exact name is traversable by an ordinary os.open. test_an_ordinary_file_still_opens_through_the_no_follow_open exercises st_nlink, lseek and dup+fdopen on the new descriptor, since on Windows it is now a CreateFileW handle in a CRT wrapper and the archive path depends on all three. test_every_pin_is_released_even_when_a_component_refuses covers the handle-leak failure mode, which would exhaust a large export rather than merely look untidy.

test/test_portability.py 76 passed / 4 skipped on Windows against real junctions. pin_directory's existing consumers re-run clean (test_platform_compat.py, test_aws_control_storage.py, test_pinned_staging.py = 383 passed / 72 skipped); the one failure there is test_version_gate_ignores_a_sitecustomize_decoy_on_pythonpath, reproduced with this branch's production files reverted to be3caf39b — an artefact of the PYTHONPATH override this box needs to run the suite, which is the very thing that test is about. mypy --platform linux: 0 errors in either changed file. black --check: clean on both.

What this still does not close, so it is not read as more than it is. The pins cover the read. They do not cover the enumeration: rglob descends a junction in order to yield what is beyond it, and is_sensitive_path/is_file() resolve by name before _open_inside is reached, so a junction already planted when the export starts is still traversed by the walk itself. That is main's behaviour, unchanged here; closing it means replacing rglob with a prunable walk, a separate change with its own regression surface. The description is re-synced to what ships.

🤖 Generated with Claude Code

@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from 5663dd3 to 651241f Compare September 7, 2026 09:43
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

The exact-head security [BLOCK-MERGE] on 5663dd30a is accepted and fixed at 651241f72. Not argued down — the finding is right, and the answer to the question it forces ("can the export enumerate these trees without resolving through an unverified component?") is yes, using a primitive the repository already had.

The finding, reproduced before anything was changed. Every resolving call was wrapped and an export run over a workspace holding one pre-planted junction:

head 5663dd30a (pinned read, rglob enumeration)
  resolving calls through the junction : 14   (4x os.path.realpath, 8x ntpath._getfinalpathname,
                                               2x os.scandir seen via audit hook)
  loot archived                        : False

head 651241f72 (pinned walk)
  resolving calls through the junction : 0
  loot archived                        : False

Both builds archive nothing from behind the junction. That is exactly why "the file was not archived" cannot be the test: if the junction names \\host\share those 14 resolutions are the outbound SMB authentication, and the refusal that follows cannot recall them. rglob descended it; is_sensitive_path(str(fpath)) and fpath.is_file() resolved what it yielded — all before _open_inside ran. This is the residual the previous body disclosed and scoped out; it is closed here rather than disclosed again.

The ordering you prescribed is what shipped. _walk_contained / _walk_pinned replace rglob:

  1. the crew root is pinned first — the kernel walks that name to reach every candidate, so leaving it out would hang the whole export off a swappable component;
  2. a directory is pinned with platform_compat.pin_directory before it is listed;
  3. children are classified from the listing itself, never by a call that resolves them;
  4. a child directory is descended only by pinning it in turn;
  5. the leaf is opened with platform_compat.open_file_no_reparse;
  6. bytes are streamed from that descriptor.

The measurement that made step 3 possible, since it is the load-bearing one and I did not want to assert it from documentation:

probe result
DirEntry.is_symlink() on a junction False — tag is IO_REPARSE_TAG_MOUNT_POINT, not ..._SYMLINK. This is why rglob walked in
DirEntry.is_dir(follow_symlinks=False) on a junction True — it looks like an ordinary directory
DirEntry.stat(follow_symlinks=False).st_reparse_tag 0xa0000003, from the listing, 0 calls naming the junction
pin_directory on a junction NotADirectoryError, 0 calls naming the junction
open_file_no_reparse on a junction OSError, 0 calls naming the junction

So _entry_is_link reads the reparse attribute off FindFirstFileW data Windows has already returned — a child's type and link-ness cost no lookup through that child. It refuses every reparse point rather than only link-shaped ones, which is precisely what pin_directory and open_file_no_reparse already refuse: it classifies what the opens would reject anyway, so it is a cheap skip and never the enforcement.

is_sensitive_path on the pathname is removed, not relocated — that call was 12 of the 14 probes. The same question is still asked in _open_verified, of the descriptor's real path, which was always the load-bearing one because a name can be re-pointed and a descriptor cannot.

_pin_ancestors and _open_inside are deleted rather than kept beside the walk. The walk holds the same chain for a whole subtree instead of re-walking it per file, so keeping both would be two mechanisms doing one job — and strictly fewer opens than before.

The regression asserts on the calls, not the archive. test_a_pre_planted_junction_is_never_resolved_during_an_export wraps os.stat, os.open, os.listdir, os.scandir, os.path.realpath/isfile/isdir/exists/getsize and ntpath._getfinalpathname, and fails if any names the junction or anything below it. os.lstat and os.path.islink are deliberately excluded — they do not follow the final component. An audit hook covers scandir/listdir as well, because pathlib binds scandir at import time and patching os.scandir cannot see rglob's own descent. Red-before, with only portability.py reverted to 5663dd30a and the new tests kept:

FAILED ...::test_a_pre_planted_junction_is_never_resolved_during_an_export
  AssertionError: the export resolved a path through the planted junction (14 calls):
  [('os.path.realpath', '...\workspace\trap'), ('ntpath._getfinalpathname', '...\workspace\trap'), ...]

test_the_recorder_actually_catches_a_probe guards that guard by making the three calls the previous build made on that exact path; it passes on both builds, as an oracle test must.

Everything you listed as preserved is pinned and green: hardlink refusal, ZIP64 streaming, descriptor-bound archive bytes, ancestor-swap protection (test_a_racing_writer_cannot_swap_a_pinned_ancestor, retargeted to the walk), the linked crew-root regression, and pin release on failure. test/test_portability.py is 78 passed / 4 skipped; pin_directory's existing consumers are 383 passed / 72 skipped with the one failure being this box's PYTHONPATH sitecustomize artefact, which does not touch platform_compat.py in this diff.

POSIX is not handed Windows semantics. The walk pins on both platforms, but only half the property is real there: pin_directory on POSIX is O_RDONLY | O_DIRECTORY | O_NOFOLLOW, whose refusal of a symlinked directory is genuine and is exactly what rglob already did — so the exported set is unchanged — while the anti-rename half has no POSIX equivalent and nothing here claims it.

The exported set is byte-identical, measured rather than argued: on a tree covering nesting, EXCLUDE_DIRS, EXPORT_EXCLUDE, .pid and skills/auto, the namelist and the manifest counts match the previous walk's exactly.

One pre-existing defect found while measuring that, and deliberately NOT fixed here. _is_excluded(PurePosixPath(str(rel))) — on Windows str(rel) uses backslashes, so PurePosixPath parses the whole relative path as a single part, and EXCLUDE_DIRS / the EXPORT_EXCLUDE basename set never match for a nested file there (workspace/notes/.env is exported on Windows). That line is untouched by this PR and the behaviour is identical on both sides of it. It is a different defect on a different path, and folding it into a security fix that has been blocked four times would make this unreviewable; it wants its own change. Flagging it rather than leaving it to be found.

Amended rather than stacked, to stay inside the two-commit rule; the PR body is re-synced to what actually ships, and the residual paragraph it used to carry is gone because the residual is.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 7, 2026
@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from 651241f to f1dc774 Compare September 7, 2026 12:59
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

Backend Tests (3.12, 3) and Backend Tests (Windows) (3) were red on 651241f72, and it is branch-caused — my own test, not a flake and not inherited. Fixed at f1dc7745a.

FAILED test/test_portability.py::TestTheArchivedBytesAreTheValidatedBytes::test_the_zip64_guard_can_actually_fail
  E  RuntimeError: File size too large, try using force_zip64
  AssertionError: Regex pattern did not match.

The cause is not a typo, and it is worth stating precisely because it hid from every local gate: CPython reworded this exception between the versions in play. 3.10 raises File size unexpectedly exceeded ZIP64 limit; 3.12 raises File size too large, try using force_zip64. My match="ZIP64" pinned the 3.10 spelling, and the only interpreter on my machine is 3.10 — so the suite was green locally and red on CI's 3.12. The test itself was always correct about the branch; it was wrong to assert on a stdlib message's wording.

The repair is one line plus the reason: match="(?i)zip64", which matches both spellings on the single token they share, keeping the assertion specific to the ZIP64 branch without pinning wording the stdlib may reword again.

'ZIP64'      -> [True, False]   # 3.10 msg, 3.12 msg
'(?i)zip64'  -> [True, True]

test/test_portability.py is 78 passed / 4 skipped; flake8 and scripts/check_black_formatting.py clean. The diff is 8 lines in that one test file — no production file touched, so nothing about the pinned-walk behaviour changed. Coverage Gate was derivative of this failure.

GPT's advisory on open_file_no_reparse's docstring is deliberately still not actioned: it carries no [BLOCK-MERGE], and touching the branch for an advisory would churn the SHA and supersede four clean exact-head reviews for nothing.

Amended rather than stacked, to stay inside the two-commit rule.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 7, 2026
`create_export_zip` walked `workspace/`, `plan_memory/` and `skills/` with
`rglob("*")`, which DESCENDS a directory link while the file on the far side
is an ordinary one, so the walk's own `is_symlink()` skip never fired for it.
Neither by-name filter below that skip is a containment test either:
`_is_excluded` is a rule about the archive name, and `is_sensitive_path` asks
whether a path is a PROTECTED location -- it does resolve links, so a linked
`~/.aws` was caught -- never whether it is inside the crew directory. An
ordinary file of the user's, reached through a link dropped in the workspace,
was sensitive to none of them and went into an archive the user hands on.

A resolve-and-compare would answer that for an instant only: `ZipFile.write`
takes a NAME and opens it again, so the file that was checked and the file
that is read are two separate lookups, and a running gateway gives agent
tools write access to that workspace while an export can be triggered.

`_open_verified` inverts the order -- open first, then ask the kernel where
the open thing is via `pinned_fs.fd_real_path` -- and `_add_from_fd` streams
the entry from that descriptor, so check and use address one object. A
hardlink alias, which no path-based guard can see, is refused on the
descriptor's link count; `force_zip64=True` keeps a source over `ZIP64_LIMIT`
archiving as `ZipFile.write` used to.

That settles what may be ARCHIVED. It does not settle what may be TOUCHED,
and on Windows those are different questions: resolving a path whose
component is a link to a UNC share IS an outbound SMB authentication, so a
refusal computed afterwards has already paid the cost it exists to prevent.
No pathname check fixes it -- every by-name check is a check-to-open window
and an adversary that can plant the link chooses when. The names have to be
held rather than inspected.

The enumeration was the last place that still inspected them. Pinning the
candidate's ancestors closed the swap race, but `rglob` had already descended
a PRE-PLANTED junction, and `is_file()` and `is_sensitive_path()` had already
resolved what it yielded, before any of it ran. Measured on this module,
exporting a workspace holding one planted junction: 14 resolving calls went
out through it -- `os.path.realpath` and `ntpath._getfinalpathname` -- while
the export correctly archived nothing from behind it. Nothing being archived
was never the question.

So the walk descends and verifies in one motion instead. `_walk_pinned` pins
a directory with `platform_compat.pin_directory` before listing it -- the
handle omits `FILE_SHARE_DELETE`, so a held directory can be neither renamed
nor deleted, nor can anything above it, and the open refuses to follow a
reparse point, so a junction at the name fails there instead of being
traversed -- then classifies each child from the listing Windows already
returned, where the attributes and reparse tag arrive inline and cost no
lookup THROUGH the child. A child directory is descended only by pinning it
in turn, so the path the kernel walks to reach any component runs entirely
through components already opened and verified, starting at the crew root
itself. This is the pattern `aws_control/backend/storage.py` already uses to
hold the path a sandboxed CLI writes through. The leaf goes through the new
`platform_compat.open_file_no_reparse`, which opens a reparse point AS ITSELF
rather than following it, so the refusal and the open are one operation;
POSIX keeps the `O_NOFOLLOW` open it already had, named.

`_pin_ancestors` and `_open_inside` are gone rather than kept alongside this:
the walk holds the same chain, for the whole subtree instead of re-walking it
per file, so keeping them would be two mechanisms doing one job.

The walk starts at the RESOLVED root, since `$KIROCREW_HOME` may itself be a
link and `pin_directory` refuses a reparse point at a name -- pinning the
configured spelling would return an empty export on such a host.

The exported set is unchanged: on a tree covering nesting, `EXCLUDE_DIRS`,
`EXPORT_EXCLUDE`, `.pid` and `skills/auto`, the archive is byte-identical to
the one the previous walk produced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@leonlaiyc
leonlaiyc force-pushed the fix/portability-export-junction-containment branch from f1dc774 to 82700d5 Compare September 7, 2026 14:06
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

The exact-head security [BLOCK-MERGE] on f1dc7745a is accepted and fixed at 82700d562. Not argued down — and worth saying plainly: this PR disclosed that defect and scoped it out, and that call was wrong.

The body argued it was a separate defect on a separate path that would make a five-times-blocked security fix unreviewable. The reviewer's answer is the better one: in a change whose entire subject is "stop the export packaging bytes it should not", shipping a known path that puts workspace/.env into the archive on Windows is not a scope boundary, it is the same bug wearing a different hat. The measurement was already in the body; only the conclusion was wrong.

The defect. _keep_for_export asked _is_excluded(PurePosixPath(str(rel))). On Windows str(rel) is backslash-separated, so PurePosixPath parses the whole relative path as ONE component: .name becomes workspace\notes\.env and .parts has length one. Both exclusion rules then stop matching.

PurePosixPath(str(PureWindowsPath("workspace/notes/.env"))).parts  -> 1 component
PurePosixPath(str(PureWindowsPath("workspace/notes/.env"))).name   -> 'workspace\notes\.env'
PurePosixPath(*PureWindowsPath("workspace/notes/.env").parts).name -> '.env'

The fix is the reviewer's, verbatim: PurePosixPath(*rel.parts) for the filter, and rel.as_posix() for the archive name. Measured end to end on the same fixture tree as the parity run in the body:

before: workspace/notes/.env, workspace/__pycache__/x.pyc, workspace/snapshots/s.md  ALL EXPORTED  (7 files)
after : excluded                                                                     (4 files)

On the archive name specifically: ZipInfo happens to rewrite os.sep today, so that half is not a live bug — it stops the member name depending on that behaviour, next to a filter that must not.

Correction to the body, since it now says something false. It claimed the exported set was "byte-identical" before and after this PR. That was true of the walk rewrite and is no longer true of the PR: on Windows this change now removes files from the archive — every nested .env, .local_secret, sel_hmac.key, telemetry_salt, *.pid, and everything under snapshots/, outbox/, uploads/, __pycache__/. That is the point of the fix, and the body has been re-synced to say so rather than leaving the old claim standing.

Regression, and it is platform-independent. TestExclusionsSurviveAWindowsSeparator drives _keep_for_export with PureWindowsPath instead of staging a real Windows export, so parts and str() differ on every host and the tests fail everywhere if the old spelling returns — this does not depend on the Windows shards to have teeth. It parametrises every EXPORT_EXCLUDE basename and every EXCLUDE_DIRS entry, keeps negative controls so the rebuild cannot start excluding everything, pins the skills/auto rule that also reads parts, and carries a guard-the-guard that reproduces the one-component parse directly.

Red-before with only portability.py reverted to f1dc7745a: 8 failed, 5 passed. Green after: test/test_portability.py 91 passed / 4 skipped. flake8, isort, mypy --platform linux (0 errors) and scripts/check_black_formatting.py clean.

Amended rather than stacked, to stay inside the two-commit rule.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@iamwhatever
iamwhatever merged commit ea9b6da into kirodotdev:main Sep 7, 2026
74 of 75 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants