Skip to content

fix: provision app requirements via pip --target, surface pip failures (#7878) - #7901

Open
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/app-requirements-target-install-7878
Open

fix: provision app requirements via pip --target, surface pip failures (#7878)#7901
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/app-requirements-target-install-7878

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

On a packaged install, provisioning an app's requirements.txt can never succeed: the bundled interpreter ships pip but no ensurepip, so the per-app python -m venv dies right after creating the directory skeleton. Neither half of the failure reaches the user -- venv creation collapses into a logger.warning, and the pip install call passes no check at all, so a non-zero pip exit is discarded without even a log line. The backend then spawns without its dependencies and dies on a ModuleNotFoundError that points at the app, not at provisioning. Worse, the half-built .venv skeleton is runnable enough that resolve_app_python prefers it afterwards, so a failed attempt actively degrades interpreter selection and never self-heals (the directory now exists, so creation is skipped forever).

Why it matters

Every third-party app that declares a requirements.txt alongside a spawned backend is dead on arrival on packaged installs -- the mainstream install path -- and the visible symptom (an import error inside the app) sends both the user and the app author debugging the wrong code. The second-order effect means even a later gateway upgrade that fixes provisioning inherits the poisoned .venv.

What changed (motivation -> approach -> change)

Symptom -> root cause: the provisioning mechanism itself requires ensurepip, which packaged interpreters do not carry, and its two error paths (checked venv, unchecked pip) both fail silent. The bundled runtime does carry a working importable pip, so pip install --target sidesteps the bootstrap entirely.

  • apps/backend.py -- the venv+pip pair becomes one sys.executable -m pip install --target <app>/.kirocrew-deps -r requirements.txt with check=True. The deps dir is prepended to the child's PYTHONPATH (honored identically by any interpreter, any platform). A provisioning failure now surfaces three ways: an ERROR log with the credential-redacted pip stderr tail, a SEL deps_provision_failed event, and a header line written into the backend's own (user-visible) log so the subsequent import error points back at provisioning. The spawn is still attempted -- the deps dir may hold a previous good install, and an offline host must not lose a working backend to a failed refresh.
  • Stamp-gated, staged install -- pip --target cannot answer "already satisfied", so a digest of requirements.txt plus the installing interpreter's ABI tag is stamped into the deps dir on success and a matching stamp skips pip entirely (a gateway Python upgrade therefore reprovisions even with an unchanged file) (no network work on restart, no false alarm on an offline restart of a healthy backend). pip fills a staging dir that is swapped live only on success, so a failed or interrupted refresh can never corrupt the prior good install in place; a crash inside the swap window itself is recovered on the next start (the outgoing tree is restored from its transient name before the stamp check).
  • apps/interpreter.py -- app_deps_dir() names the layout for both spawn paths. When the gateway has provisioned deps, resolve_app_python returns sys.executable unconditionally (those wheels are ABI-bound to it). Without provisioned deps it prefers an app-shipped .venv only on a probe that positively confirms the venv is usable -- its interpreter starts, reports a sys.prefix inside the venv, and matches the gateway's Python minor version. A pyvenv.cfg version check was not enough in either direction (a failed venv skeleton passes it; a working cross-minor venv fails it), so the probe is positive evidence, not a heuristic. venv_provided_command mirrors that precedence and probes <deps dir>/bin (Scripts on Windows) where --target places console scripts, so bare console-script commands in app manifests stay resolvable.
  • Module-style builtins are outside the boundary. A builtin whose backend runs kiro_crew package code is spawned from a writable app dir; it never provisions a requirements.txt found there and never gets .kirocrew-deps on its PYTHONPATH, so agent-authored wheels cannot load ahead of the trusted module.
  • apps/bridges.py -- stdio MCP server registration exposes the deps dir through PYTHONPATH the same way the backend spawn env does; a --target install carries no interpreter, so the env is the only bridge for python-launcher servers and deps-provided console scripts (whose shebang is the installing interpreter).
  • apps/manager.py -- .kirocrew-deps (and its transient staging/prior siblings) join the app-copy denylist next to .venv, so install/update never drags a machine-specific wheel tree along to shadow the destination's own provisioning.
  • Docs -- docs/app-kit/manifest-reference.md (stdio command resolution) and docs/app-kit/publishing-guide.md (excluded directories) updated in step with the behavior.

Tests

All in existing suites, exercising the spawn body hermetically (no real pip, no real processes):

  • test_apps_backend_coverage.py::TestDependencyInstall -- provisioning is a single pip --target into staging with no -m venv anywhere; the pip argv is sys.executable -m pip (never a bare or venv-relative interpreter); check=True is actually passed; success swaps staging live and stamps the digest; the digest changes with the interpreter ABI tag; an interrupted swap is recovered on the next start (pip skipped, prior tree back on PYTHONPATH); an unchanged requirements file skips pip; a changed one reinstalls; a failed reinstall leaves the prior deps dir intact and still on the child's PYTHONPATH; a failure logs at ERROR and writes the header into the backend log; the deps dir lands first on the child's PYTHONPATH, and no dir means no injection.
  • test_apps_backend_coverage.py::TestInterpreterResolution -- a real venv is preferred; a bootstrap skeleton (runnable system-python interpreter, current-minor pyvenv.cfg) is rejected by the probe; a venv with no interpreter falls back to sys.executable; provisioned deps pin sys.executable; deps-dir console scripts resolve, with a usable venv winning on a name collision only when no deps dir was provisioned. TestDependencyInstall adds a module-builtin case asserting no provisioning and no deps-dir PYTHONPATH injection.
  • test_app_bridges.py::TestStdioDepsDirExposure -- stdio registrations get the deps dir prepended to PYTHONPATH (manifest env preserved), only when the dir exists; a deps-provided console script is rewritten to its absolute path and gets the env.
  • test_app_manager.py -- the copy denylist drops .kirocrew-deps while runtime payload survives.

Local gates: isort, flake8, black/brand/encoding/docs-lint scripts, mypy (clean on changed files), and the full related pytest files (897 tests) all green.

Manual verification

Verified empirically that pip install --target places console scripts in <target>/bin with a shebang pointing at the installing interpreter (the fact the PYTHONPATH bridging and script resolution rest on). A live packaged-install run was not performed in this environment; the issue's own reproduction (import ensurepip failing in the bundled interpreter, pip importable) pins the platform facts the fix relies on.

Pattern harvest

Rule candidate: semgrep
Pattern: a subprocess call on a provisioning/setup path with capture_output=True, no check=, and an unused result -- the non-zero exit silently reads as success and the failure surfaces later as an unrelated error (here: pip's exit was discarded and the symptom was an import error inside the app).

Rule candidate: recurring-defect-patterns (AUTOSDE)
Pattern: an "is provisioned/ready" predicate satisfied by an artifact a FAILED bootstrap also creates (the half-built venv skeleton was runnable, so the venv-first interpreter policy preferred it). The fix shape is a positive success marker written only after the operation completes (the stamp file this PR adds), never existence/runnability of the output directory.

Closes #7878


Transport and lifecycle details (per First Principles review)

  • Transport is a launch shim, not bare PYTHONPATH. Python launches route through deps_boot (a stdlib-only shim that site.addsitedirs the deps dir before dispatching the real target), because PYTHONPATH never processes .pth files - editable installs and console scripts require it. The shim strips the deps dir from PYTHONPATH it injects elsewhere (shim XOR PYTHONPATH). Non-python and foreign-interpreter launches keep the plain PYTHONPATH transport or none at all (ABI-gated). Windows launcher pairs and embedded-ZIP console scripts have dedicated shim arms.
  • Provisioning timing. pip runs in the backend spawn path; for apps with NO backend (stdio-only MCP servers) and for adopted file-entry backends, provisioning runs at MCP registration - otherwise nothing would ever install their requirements.
  • Uninstall quarantine apparatus. The generated deps tree lives under app-writable data/ (so app updates keep the last good install). That placement makes uninstall's sweep of GENERATED artifacts a security boundary: the sweep quarantines and removes exactly the gateway-generated names (strict matcher), through pinned descriptors, and preserves everything app-owned. The stop/pidfile lifecycle machinery that previously accompanied this was extracted to issue App backend stop detection treats pidfile absence as proof of termination #9396.
  • Uninstall path-safety guard: uninstall_app validates the app name before any metadata read (_check_path_safety), unchanged from main.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound root-cause fix, but the 60s pip budget contradicts the PR's own minutes-scale lock design, and the app-writable placement creates the TOCTOU surface the PR then spends ~1k lines defending.

Watch

  • The lifecycle flock exists because "provisioning (pip can run for minutes)", yet the pip run is killed at timeout=60 (run_limited(pip_cmd, check=True, ..., timeout=60)). Any requirements set needing a source build or slow network still fails provisioning on every start — now loudly, burning 60s under the spawn lock each time — so a real class of the "dead on arrival" apps this PR exists to revive stays dead, and the waiter/flock machinery guards a duration the timeout makes impossible.
    Clears when: the pip timeout is raised or made proportionate to install size (npm nearby already gets 120s), or the minutes-scale flock rationale is corrected to match.
  • Placing the gateway-generated wheel tree inside app-writable data/ is what necessitates _PinnedDir, dir_fd renames, O_EXCL marker writes, staging pins, the quarantine-rename uninstall sweep, and the uninstall↔provision lock dance. A gateway-owned location outside the app-writable tree (exposed read-only to the app sandbox) would remove the attacker's write access to the whole transaction and delete most of that machinery; the stated reasons for data/ (survives update_app, same-filesystem renames) hold there too. No alternative placement is discussed.
    Clears when: the author documents why in-tree placement is required (e.g. the app sandbox cannot mount a gateway-owned dir read-only), or moves the tree out of the app-writable namespace.

Suggestions

  • Record the new multi-module on-disk contract (.kirocrew-deps layout, stamp/ABI activation semantics, shim-XOR-PYTHONPATH rule) in docs/system-specs/modules/app-kit-platform.md — four modules plus uninstall must stay agreed on it, and today it lives only in scattered comments.
  • Memoize _venv_is_usable per app for one spawn/registration transaction: it launches a sandboxed subprocess with a 10s timeout and can run several times per resolution (resolve, ABI match, console-script lookup).

[DESIGN-REVIEWED] 09111ad

@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 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/backend.py:1115 -- function-local import threading here and in apps/interpreter.py:129 violates top-level-imports -> Fix: use top-level imports.
[GPT-REVIEWED] 09111ad

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 09111ade86fab3bb0217f635836baca3a91b4252 — 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

A 170-line exec-capable launch shim (deps_boot.py) ships undeclared while the description claims the transport is plain PYTHONPATH "honored identically by any interpreter, any platform".

Not justified as shipped

  • Item 6 — undeclared: description says "The deps dir is prepended to the child's PYTHONPATH"; the diff says the opposite — "Shim XOR PYTHONPATH, never both" (backend.py:2132). The shim's harm (.pth files unprocessed on PYTHONPATH) is real and derived, but nothing in the intent declares it, its -c re-exec arm, or its Windows embedded-exe ZIP dispatch.
  • Item 7 — undeclared: bridges now rewrites one path-carrying command (ABI-shebang absolute script → shim launch, bridges.py:2117), yet the shipped user doc still asserts the old pin: "A command carrying a path (absolute or relative) is never rewritten" (docs/app-kit/manifest-reference.md:66) — doc and code contradict within this PR.
  • Item 8 — rides along, undeclared: uninstall gains ~200 lines of deps-tree quarantine/purge/restore (manager.py), including a new fail-loud mode where uninstall refuses. Boundary-derived (revoked .pth code surviving into a reinstall), but the description's manager.py bullet mentions only the copy denylist.
  • Item 9 — rides along, undeclared: spawn waits no longer clear a slow in-flight spawn; new per-app lock files under config_dir()/app_backend_locks. Derived from pip lengthening spawns, never mentioned.

What this change ships

Inventory (10 items) — 6 justified

Intent: make apps that declare a requirements.txt actually get their dependencies on packaged installs, and make provisioning failures visible — a FIX (issue #7878, reproduction stated).

  1. Apps with requirements.txt now work on packaged installs (pip --target replaces venv) — justified
  2. Provisioning failures surface: ERROR log, SEL event, header in the app's own log — justified
  3. Restart with unchanged requirements skips pip; volatile lines reprovision every start — justified
  4. A failed or interrupted install can no longer corrupt the last good install — justified
  5. Provisioned apps always run the gateway interpreter; an app venv is used only after an executed probe — justified
  6. Python children launch through the new deps_boot shim, not plain PYTHONPATH — undeclared (description claims PYTHONPATH transport)
  7. One path-carrying MCP command shape is now rewritten (ABI-shebang script) — undeclared; shipped doc still pins "never rewritten"
  8. Uninstall purges gateway-generated deps trees from preserved data and can now fail loud — rides along, undeclared
  9. A slow spawn's placeholder is no longer cleared at the 20s wait (flock probe; new lock files) — rides along, undeclared
  10. Install/update copy excludes .kirocrew-deps* — justified

Watch

  • Framing vs diff: "prepended to the child's PYTHONPATH (honored identically by any interpreter, any platform)" is contradicted by backend.py:2132's "Shim XOR PYTHONPATH, never both". Clears when: the declared behavior names the shim as the primary transport.
  • manifest-reference.md:66 still states "A command carrying a path … is never rewritten" while bridges.py:2117 rewrites ABI-shebang scripts. Clears when: that doc line carries the same exception the resolver docstring does.
  • Rewritten pin: the old _await_inflight_spawn comment cleared stale placeholders so a hung spawn body wouldn't wedge the app "until a gateway restart"; a hung-but-alive body now holds the flock, so that exact wedge returns. Clears when: a hung-body path that releases the flock is shown, or the wedge is bounded.
  • Windows premises (embedded-exe launcher = PE + appended ZIP with __main__.py; distlib sh-trampoline shape) are unverified — author states no live packaged run; all arms fail open to direct launch. Clears when: confirmed on a Windows packaged install.

Subtractions

  • Merge _capped_probe_spill (interpreter.py:122) into _capped_spill (backend.py:1100) — 2 copies of the same watchdog-truncate helper added in one PR (grepped def _capped.*spill: 2 definitions); keep one.
  • Defer deps_boot's Windows embedded-exe arm plus _zip_has_main (bridges.py) — without it an embedded launcher gets exactly the pre-PR direct launch, and the arm rests on the unverified vendor format above.

[FIRST-PRINCIPLES-REVIEWED] 09111ad

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 09111ad

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

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

@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch from 0f07927 to 8e0b908 Compare September 2, 2026 15:50
@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 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch from 8e0b908 to a77000e Compare September 2, 2026 16:50
@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 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch from a77000e to 3ec9bb9 Compare September 2, 2026 17:47
@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 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch 6 times, most recently from b4987a3 to f35246d Compare September 2, 2026 22:46
@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 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch from 4003f14 to 792560c Compare September 4, 2026 02:57
@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 4, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Escalation: round 18 trips the non-convergence tripwires (maintainer decision required)

The automated pipeline has fixed or dispositioned every finding through 17
GPT rounds (plus Opus, Design, First Principles, and UX lanes, all clean or
dispositioned). Round 18 landed three new blockings, and two of them trip
the structural-escalation rules the pipeline runs under, so it is stopping
rather than patching again.

Round-18 findings and assessment:

  1. BLOCKING (manager.py:1028, VERIFIED REAL): the round-17 uninstall purge
    uses rmtree(ignore_errors=True), which silently refuses a SYMLINK at
    data/.kirocrew-deps -- a malicious app can plant one and its target
    survives uninstall. The one-line fix (lstat + unlink for links/junctions
    before rmtree) is ready. NOT applied because this is the third
    consecutive round on the deps-lifecycle span (14: relocate under data/;
    17: purge on uninstall; 18: purge is link-bypassable) -- each fix has
    seeded the next, which is the defined signal that the mechanism, not the
    guard count, is wrong.

  2. BLOCKING (backend.py:1034, DESIGN DECISION): packages that ship a regular
    .pth file install fine into the --target dir but PYTHONPATH never
    processes .pth, so imports that rely on it fail at runtime. This
    generalizes round 13's editable finding to the whole .pth family, and
    GPT's prescription is now "revert --target provisioning". The pipeline
    will not adjudicate that.

  3. BLOCKING (backend.py:1087, real but small): redact_credentials catches
    user:pass@ URL forms but not signed/tokenized query strings in pip
    stderr (X-Amz-Signature, ?token=...). Mechanical fix (strip query
    strings from URLs in the redacted tail) ready alongside finding 1.

Structural options for the maintainer:

A. Launch-shim approach: spawn provisioned backends through a tiny stub
that calls site.addsitedir() before the entry point, instead
of raw PYTHONPATH. Processes .pth files correctly (kills the whole
finding family from rounds 13/18), keeps the --target mechanism and all
hardening from rounds 1-17. Roughly 40-60 lines plus tests -- above the
pipeline's autonomous-change budget, straightforward for a human or an
explicitly re-scoped pipeline run.

B. Documented-refusal approach: extend the existing editable refusal to any
top-level .pth artifact in staging (loud provisioning error naming the
package; apps that need such packages use a .venv). Small (~10 lines),
over-blocks a minority of legitimate packages.

C. Accept GPT's revert prescription and abandon --target provisioning --
returns to the original issue #7878 breakage on packaged installs; not
recommended.

Under any option, finding 1's link-aware purge and finding 3's redaction
tightening should land as written; they are queued in the worktree
(/tmp/kc-fix-7878, intact) and can be applied on instruction.

Current state on head 792560c: every lane
green or in flight except the GPT verdict above and the Code Review lane's
recurring npm-audit 120s timeout (infra flake, 6th occurrence, re-run
pending).

Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c]

@chenmingwei23
chenmingwei23 force-pushed the fix/app-requirements-target-install-7878 branch from 792560c to 3ee8e39 Compare September 4, 2026 06:18
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round-18 resolution on head 3ee8e39
(operator selected structural option A from the escalation above):

  1. Launch shim (new src/kiro_crew/apps/deps_boot.py): python backends and
    python-launcher stdio servers with provisioned deps now spawn through
    python -m kiro_crew.apps.deps_boot <deps_dir> <target>, which
    site.addsitedir()s the deps dir -- processing .pth files (editable
    hooks, namespace shims, import hooks) that PYTHONPATH never processes --
    moves the added entries to the FRONT of sys.path (preserving the
    deps-win precedence the PYTHONPATH transport had), and runs the target
    with an unchanged argv view. This removes the root cause of the
    round-13/18 .pth finding family instead of guarding it again. The shim
    is used only when the child runs the gateway's own interpreter (deps pin
    sys.executable); console scripts and path-based commands keep the
    PYTHONPATH transport, and interpreter-option-first args fall back to it
    rather than being misread. End-to-end test: a package reachable only
    via a .pth redirect imports under the shim in a real subprocess.

  2. Link-aware uninstall purge (manager.py): the round-17 purge now unlinks
    a symlink/junction planted at any data/.kirocrew-deps* name before
    rmtree, so the link -- not its target -- is removed and revoked code
    cannot ride an uninstall-reinstall back onto PYTHONPATH. Test pins that
    the link is gone and the linked target elsewhere is untouched.

  3. Query-string redaction (backend.py): signed/tokenized URLs in pip
    stderr (X-Amz-Signature, ?token=...) now have their query strings
    stripped, applied to the FULL stderr before the 400-char tail cut so a
    truncation can never split the URL from its query and leak the token's
    tail. Test pins the split-safety.

All lanes re-roll on this head.

Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c]

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is OVERLAPPING with PR #6599. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7901: CONTINUE_DEVELOPMENT. Independent goals in one shared function. Keep both; sequence them and expect a one-hunk conflict resolution in the spawn-env block for the second to land. Files: src/kiro_crew/apps/backend.py.

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

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Escalation: convergence test failed at round 21 (pipeline stopping for a maintainer)

Per the operator's hard line, round 21 was the convergence test for the
automated pipeline's rounds 12-20. It landed two new blocking findings on
pipeline-written code, making TEN consecutive rounds in which each round
found real defects in the previous round's own fixes. The pipeline is
stopping for good; the remaining tail belongs to a human.

Full round history:

Round Head Findings Outcome
1-3 0f07927..a77000e 7 fixed
4 3ec9bb9 clean -
5-9 19080b0..2390ade 6 fixed; 1st+2nd escalations, operator resumed
10 03a8651 1 (probe env) operator-approved fix
11 8cc0af3 1 (path-cmd deps) fixed under standing authorization
12 428462d 2 (TOCTOU, pip cwd) fixed
13 1705c92 2 (temp-copy includes, editable .pth) fixed
14 a5112c9 2 GPT + FP BLOCK 2 fixed, 1 disproved (probe), rider reverted
15 7f0eb77 1 (include stamp) fixed
16 4003f14 3 (volatile forms, first-enable env, Scripts dir) fixed
17 792560c 1 (deps survive uninstall) fixed
18 3ee8e39 3 3rd escalation; operator chose option A (shim)
19 98a6cb0 3 (isjunction, silent purge, in-tree links) fixed
20 2da8c9b 3 (run_path sys.path, stale trees, purge ordering) fixed
21 (current) 2, below STOPPED

Open round-21 findings (both look real, both small):

  1. backend.py:463: the volatile-requirements detector misses BARE relative
    paths (wheels/pkg.whl) that start with neither ./ nor / -- an updated
    local wheel with unchanged requirements.txt reuses the stamp. Fix shape:
    also treat any non-option line containing a path separator as volatile.
    This is the FOURTH round on the stamp-reuse span (13/15/16/21); the
    structural alternative is dropping stamp-gating entirely and
    reprovisioning on every start (one pip no-op run per start, network
    permitting -- the stamp exists to keep offline restarts quiet, so
    dropping it needs a product call).

  2. bridges.py:2121: the attached -mMODULE spelling falls back to PYTHONPATH
    transport and skips .pth processing. Fix shape: normalize -mMODULE to
    the separate form and shim it.

Everything else on head 2da8c9b is green
or in flight with no failures; Opus/Design/FP/UX lanes clean or
dispositioned throughout.

What the pattern says: the mechanism (stamp inference + argv rewriting) has
a long tail of forms, and a non-deterministic reviewer that re-rolls on
every push will keep finding one form per round. A maintainer reviewing the
whole surface once will converge faster than this loop can.

The branch and worktree (/tmp/kc-fix-7878) are intact. The two fixes above
are small and well-understood if a maintainer wants them applied as-is.

Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c]

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Maintainer handoff (final automated round)

On operator instruction, the two round-21 findings are applied as this
pipeline's FINAL round, on head ece06bf:

  1. Volatile-requirements detection now treats any non-option line carrying
    a path separator as volatile (covers bare relative paths like
    wheels/pkg.whl), so an updated local artifact can never be masked by a
    matching stamp. Over-matching an exotic marker line costs one redundant
    pip run.
  2. The attached -mMODULE spelling is normalized to the separate form before
    the shim walk (CPython treats them identically), so it now routes
    through deps_boot and gets .pth processing instead of silently falling
    back to the PYTHONPATH transport.

Both pinned by tests; all local gates green (482 tests, black/flake8/
isort/mypy).

The automated pipeline is now DONE on this PR and will not push again.
Rationale in the round-21 escalation above: ten consecutive review rounds
each found real defects in the previous round's fixes, so a human review
of the whole provisioning surface will converge faster than this loop.
The review lanes will re-roll on this head; whatever they land belongs to
the human tail, along with the standing product question of whether to
keep stamp-gating at all (dropping it and reprovisioning every start
removes the largest finding-span outright).

Branch and worktree are intact. Summary of what this PR now ships is in
the description; the round-by-round history is in the escalation table
above.

Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c]

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 68: structural close-out of the by-name-write family (head 2bd098a)

Per the flagged finding on 3625eab (marker writes redirectable by a staging swap after pip), this round enumerates EVERY gateway write in the provisioning transaction against one criterion and fixes the family at once, instead of point-fixing the flagged pair.

Family criterion: a GATEWAY write, executed AFTER app-runnable code (pip build hooks) has had write access to data/, addressed to an app-writable path BY NAME rather than through a held descriptor.

# Site Shape before Verdict Shape now
1 stamp write (_DEPS_STAMP_NAME) atomic_write(staging / name) by name, post-pip IN family (the flagged pair) written through the staging descriptor pinned at creation: O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW with dir_fd (POSIX); verify() + atomic_write floor on Windows (_write_staging_marker)
2 ABI marker write (_DEPS_ABI_NAME) same IN family same
3 publish rename (pin.rename(staging.name, deps_dir.name)) dir_fd rename, but the ENTRY at staging.name was never re-verified post-pip IN family (adjacent: publishes whatever sits at the name) staging_pin.verify() (held fd vs fresh lstat identity) immediately before the swap
4 requirements snapshot write pinned-parent walk + O_EXCL|O_NOFOLLOW (pre-pip) already correct - this was the template unchanged
5 staging mkdir os.mkdir(dir_fd=pin.fd) already correct (creation precedes any hook code) unchanged; now immediately followed by _PinnedDir(staging) so the identity is held for the whole transaction
6 stale-staging sweep descriptor-relative enumerate + quarantine rename + delete already correct (rounds 58/59) unchanged
7 prior-tree renames + removals in the swap dir_fd within the pinned parent (_pinned_remove_entry, pin.rename) already correct unchanged
8 failure-arm staging quarantine _pinned_remove_entry already correct unchanged (staging pin closed first)
9 deps_dir.exists() / prior.exists() branch probes by-name READS choosing between pinned renames out of family: reads, and the worst case is a failed rename (ENOENT), never a foreign write unchanged
10 pip --target <staging path> the pip CHILD writes by path out of family: the child runs at app privilege in the sandbox - a swapped staging redirects the APP's own writes, not the gateway's; pin.verify() still precedes handing out the path unchanged
11 backend log writes (log_fh) gateway-owned logs dir out of scope: not app-writable unchanged
12 pidfile / lock writes config_dir(), gateway-owned out of scope unchanged
13 uninstall purge / quarantine (manager.py) descriptor-pinned since earlier rounds already correct unchanged

Windows note: items 1-3 keep the transaction's existing Windows floor (no dir_fd there): _PinnedDir captures the directory identity (volume serial + file index) at creation and verify() re-checks it plus the junction pre-check before each by-name step - the same accepted floor as every other Windows arm of this transaction.

New regression test: test_a_staging_swap_after_pip_cannot_redirect_marker_writes_or_publish simulates a build hook renaming staging aside and planting a symlink to a victim directory after pip exits - provisioning fails loud, no marker lands in the victim, and the swapped tree is never published live.

Context for reviewers: the tree at 3625eab was byte-identical to 6d99f4b, where both AI lanes were green; the finding is by construction pre-existing and was judged REAL (reachable, matching the defense the snapshot write already carries), hence fixed rather than overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 69: close-out of the untrusted-command-path surface in bridges.py (head 1103380)

Both flocking findings on 2bd098a are fixed, and per the same discipline as the staging-write table (comment 5578049270), here is the full enumeration of every point in resolve_stdio_command's orbit where a manifest/command path resolution result feeds a FILE OPERATION or PROCESS LAUNCH, with the gate verdict for each.

Family criterion: a manifest-author-controlled path (or a value derived from one) reaching a gateway-side file read/write or influencing what gets executed, without passing the sanctioned gate for that class (is_sensitive_path for content reads outside the sandbox; app-root-confined construction for probes; the sandbox for execution).

# Site Operation Verdict Gate now
1 _python_shebang_interpreter(name) bounded 512B content READ of a manifest-verbatim absolute path WAS IN family (flagged F1) is_sensitive_path refusal BEFORE open; refusal reads as None (direct launch)
2 shebang argument handling in the same reader interpreter REWRITE dropped #!<python> -I flags WAS IN family (flagged F2) an argument-bearing shebang is never a rewrite candidate (len(parts) != 1 -> None): the kernel passes shebang arguments as one token with its own splitting rules, and isolation flags (-I/-E/-s) set a posture a rewrite cannot carry faithfully - the direct launch keeps every flag exactly as written
3 _has_python_shebang(venv_cmd) bounded content READ defense-in-depth: callers pass app-dir-confined constructed paths today same is_sensitive_path refusal added, so the reader contract holds against any future caller
4 _zip_has_main(venv_cmd) inventory vet + ZIP READ same as #3 same gate added (in addition to the existing vet_zip_inventory member cap)
5 zipfile.is_zipfile(venv_cmd) tail READ out of family: venv_cmd comes only from venv_provided_command, which requires a bare name (caller-enforced: separators/drive qualifiers take the path arm) and joins it under the app's own venv/deps dirs construction-confined
6 venv_provided_command(root, name) stat/executability PROBES out of family: bare-name joins under app root; metadata only, no content read construction-confined
7 path_command_is_abi_matched(app_root, name) resolve(strict=True) + identity comparison out of family: metadata resolution only, no content read, no execution; relative names are refused outright metadata-only
8 _venv_is_usable probe EXECUTES the app venv python out of family: runs under the same OS sandbox + resource ceilings as every app spawn, bounded output capture sandboxed execution (the sanctioned path)
9 the resolved command's eventual spawn EXECUTES the manifest command out of family: MCP stdio servers launch under the app sandbox; resolution here only picks the argv, never escalates the execution context sandboxed execution
10 _deps_tree_stamp_current(app_root, ...) marker READS out of family: reads confined to the app root with containment check + component-pinned no-follow opens pinned reader
11 _maybe_provision_backendless_deps pip install out of family: delegates to the provisioning transaction (parent + staging pinned, see the round-68 table) pinned transaction
12 _normalize_attached_m / _py_target_index / _strip_deps_pythonpath argv/env munging out of scope: no filesystem contact n/a

Regression tests added: a sensitive-path command is refused by all three readers BEFORE any open() (tracked via a monkeypatched builtins.open), and a #!<python> -I script keeps its direct launch with no deps_boot wrap.

Also in this push, per the new lint ratchet and the ASCII branch rule: every added comment/docstring line narrating change history is rewritten to present-tense behavior, and all added lines are printable ASCII (em dashes normalized). No functional change beyond the two fixes above.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Scope restoration: extracting the stop/pidfile lifecycle machinery (issue #9396)

The next push SHRINKS this branch substantially. This is a planned extraction, not an abandonment - the boundary, the reasons, and where everything goes:

What STAYS (the PR's purpose, #7878)

  • The provisioning transaction: pip --target into a pinned staging dir, requirements snapshot, stamp + ABI markers written through the held staging descriptor, identity-verified publish rename, stale-staging sweeps with the strict generated-name matcher
  • deps_boot and the full stdio command resolution in bridges/interpreter: shim arms, stamp-gated deps exposure AND selection, shebang readers with the sensitive-path gate, ABI matching
  • Uninstall's generated-artifact purge with descriptor-pinned quarantine, and preserved-data protection (app-owned prefix-sharing names survive; failed purge restores data)
  • pip failure surfacing with credential-safe redaction

What LEAVES (tracked in #9396, to be REDONE - not ported - in a follow-up PR)

  • stop_recorded_app_backend / _stop_recorded_outer / _stop_recorded_locked / stop_recorded_backend_preflight
  • The pidfile hardening built for them: _pidfile_flock, _forget_app_pid_if, adopted-backend record persistence, record-write-failure spawn teardown, the conditional-forget semantics in stop_app_backend
  • The stop preconditions wired into uninstall_app, the CLI uninstall arm, and the dashboard uninstall handler (Step 0.5 preflight + Step 3 confirmed stop)

Why

Review produced 11 real findings in this family, all facets of one design decision: treating the ABSENCE of a same-user-deletable pidfile as proof of termination. Point fixes cannot close that class (a 12th instance should be assumed); issue #9396 records the root cause and the positive-evidence design the follow-up must use ("absent reads as UNCONFIRMED, not stopped").

Resulting behavior

After extraction, uninstall behavior equals MAIN'S CURRENT BEHAVIOR plus the generated-artifact sweep - no capability that works on main is lost. What leaves is an in-progress capability main never had; its absence is now tracked in #9396 rather than half-shipped here.

The per-app lifecycle flock itself stays: the spawn path holds it and the in-flight-spawn waiter probes it, which is what keeps slow pip provisioning (the core of this PR) from breaking spawn single-flighting.

@bolichen97

Copy link
Copy Markdown
Collaborator

@chenmingwei23 Thanks for this, and for staying with a long review loop. We audited it at 6d99f4b; your branch has moved since, so I have kept this to claims the new head cannot invalidate.

Nothing on main covers any part of it. src/kiro_crew/apps/backend.py on main still spawns sys.executable -m venv for requirements.txt and calls pip with no check=, and resolve_app_python in src/kiro_crew/apps/interpreter.py still prefers .venv/bin/python3 on a bare runnable check. On main, git grep returns zero hits for every symbol you add: provision_app_deps, app_deps_dir, .kirocrew-deps, deps_boot, stop_recorded_app_backend, _venv_is_usable, path_command_is_abi_matched, .requirements-sha256. The closest merged work, #7978, only adds an in-process gateway-shutdown sweep. So the fix for #7878 is still entirely missing and worth landing.

Two things before it can merge.

Scope. At 6d99f4b roughly a third of the diff was an undeclared cross-process stop and uninstall lifecycle subsystem: stop_recorded_app_backend, adopted-pid persistence, the pidfile flock, uninstall 409s in src/kiro_crew/apps/routes.py, the CLI abort in src/kiro_crew/cli_commands.py, and the quarantine and restore transaction in uninstall_app. That changes uninstall behaviour for every app. Your current head no longer touches routes.py or cli_commands.py, which is the right direction. Please confirm the remaining pieces are also split into their own PR and leave this one at the provisioning fix.

Rebase. The audited base was 712 commits behind main. Expect a one-hunk conflict with #6599 and #8778 in _start_app_backend_body, and with #5488 in handle_uninstall_app if any uninstall change stays.

Your closing question, stamp gating versus reprovisioning on every start, still needs a maintainer answer. I will come back to you on it.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Adjudication of the blocking finding on df71777 (manager.py survivor-scan race)

The finding is REAL as stated: a backend that outlives uninstall can write a generated tree after the survivor scan and before the data restore. It is also, by its own premise, the extracted family: the race exists only because "CLI uninstall leaves the backend running" - which is item 1 of the stop/pidfile lifecycle family moved out of this PR per the posted scope-restoration boundary (see the extraction comment) and tracked in issue #9396. Every fix shape available here (stop and verify the backend, refuse preservation while it can write) rebuilds exactly the machinery that was extracted; the durable record in #9396 already names this consequence explicitly ("the original bug returns to UNFIXED... this issue is its only tracker").

Out-of-scope for this PR, not a false positive.

/ai-review override gpt df71777: out-of-scope - the race requires a backend that uninstall no longer stops, which is the stop/pidfile lifecycle family extracted from this PR per the posted boundary and tracked in issue 9396

Also noting the two advisory doc contradictions (bridges.py:2023 bare-name wording, bridges.py:2638 absent-backend wording) - both real, both non-blocking; they ride along with the next push that is otherwise required rather than re-rolling every lane for comment-only changes.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

@bolichen97 Thank you for the audit - and for confirming the #7878 fix is still missing on main and worth landing.

Scope: confirmed, with pointers. The lifecycle subsystem you identified was extracted from this branch on 2026-09-08 (before your comment landed, which is why the head moved past 6d99f4b):

  • Removed entirely: stop_recorded_app_backend and its preflight, adopted-pid persistence, the pidfile flock, the uninstall 409s in routes.py, the CLI abort in cli_commands.py, and the stop preconditions in uninstall_app. The entangled functions (stop_app_backend, _record_app_pid, the stale-reap) were restored to main's shapes verbatim, not hand-edited.
  • The extraction boundary and rationale are posted above (the "Scope restoration" comment), and the design defect that motivated the whole family - pidfile ABSENCE treated as proof of termination - is recorded in issue App backend stop detection treats pidfile absence as proof of termination #9396, including an explicit note that uninstall-leaves-the-backend-running RETURNS TO UNFIXED and that the follow-up must be a rebuild on positive evidence (absent record reads as UNCONFIRMED), not a port of this branch's history. The follow-up PR does not exist yet; App backend stop detection treats pidfile absence as proof of termination #9396 is its tracker.

Rebase: done. The branch was rebased onto current main on 2026-09-08 (clean, no conflicts - the extraction had already removed the pieces your conflict prediction pointed at). Current head df71777405bb05b35f3dc339fcf042c08ae16e91 sits on top of e7db5f5b8-era main.

One scope question for you. The single uninstall-adjacent piece still in this branch is the generated-artifact purge in uninstall_app: it quarantines and removes exactly the .kirocrew-deps* trees THIS PR creates under app-writable data/ (strict name matcher, descriptor-pinned, app-owned names preserved, data restored on failure). We kept it because the trees are this PR's own product - without the purge, a keep-data uninstall preserves gateway-generated .pth code into data a same-name reinstall re-activates, which review flagged repeatedly. But if you read it as part of the lifecycle subsystem, we can split it into the #9396 follow-up too and accept the interim gap. Your call - either shape is a small change from the current head.

On stamp gating versus reprovision-on-every-start: standing by for your answer; the current head stamps requirements bytes + interpreter ABI and falls back to an ABI-only marker so a failed refresh keeps serving the last good tree.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt df71777: out-of-scope - the race requires a backend that uninstall no longer stops, which is the extracted stop/pidfile lifecycle family tracked in issue 9396 per the posted scope-restoration boundary

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for df71777405bb05b35f3dc339fcf042c08ae16e91.

out-of-scope - the race requires a backend that uninstall no longer stops, which is the extracted stop/pidfile lifecycle family tracked in issue 9396 per the posted scope-restoration boundary

This decision applies only to this commit. A new push requires a new judgment.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

@bolichen97 Closing out the one open scope question from my earlier reply: the generated-artifact purge in uninstall_app stays in this PR.

The reasoning is ownership, not boundary preference: the purge removes exactly the .kirocrew-deps* trees this PR's provisioning creates, so it belongs to whichever change creates the trees. Moving it to the follow-up would make #7901 ship a known leak - a change that adds artifacts and declines to remove them is incomplete on its own terms, whatever a later PR does. And #9396 is deliberately scoped to one design question (replacing absence-as-stopped with positive evidence in stop detection); artifact purging shares a function with that machinery but not the design question, and keeping the follow-up issue focused is what keeps it actionable.

The stamp-gating question stays with you; the current head works either way.

#7878)

Packaged installs bundle an interpreter with pip but no ensurepip, so the
per-app venv provisioning died after creating the directory skeleton --
which the venv-first interpreter policy then preferred while it held no
dependencies. Replace the venv+pip step with a single
'sys.executable -m pip install --target <app>/.kirocrew-deps' (no
bootstrap needed, works under packaged and source installs) and expose
the deps dir to the child via PYTHONPATH -- on the backend spawn env and
on app stdio MCP server registrations alike.

The pip call now passes check=True (a non-zero exit was silently
discarded) and a provisioning failure is surfaced: an ERROR log with
credential-redacted pip stderr, a SEL event, and a header line in the
backend's own log so the import error missing deps produce points back
at provisioning instead of reading as an app bug. The spawn is still
attempted -- the deps dir may hold a previous successful install.

The install is stamp-gated and staged: a digest of requirements.txt plus
the installing interpreter's ABI and platform tags, stamped on success,
skips pip entirely on unchanged restarts (no network work, no false
alarm on an offline restart of a healthy backend) while a gateway Python
upgrade or a cross-architecture migration still reprovisions; pip fills
a staging dir swapped live only on success, so a failed or interrupted
refresh can never corrupt the prior good install in place, and a crash
inside the swap window is recovered on the next start.

Because provisioned deps are built by sys.executable, an app with a
provisioned deps dir always runs under sys.executable -- a venv can
match the minor version and still differ in architecture, which no
pyvenv.cfg field can rule out. Without provisioned deps an app venv is
preferred only when its pyvenv.cfg names the gateway's own Python minor
version. Console scripts from requirements.txt land in <deps dir>/bin
(Scripts on Windows) under --target, so venv_provided_command probes
there with the same precedence, keeping bare console-script commands in
app manifests resolvable. A server launching a kiro_crew module never
receives the app deps on PYTHONPATH, so an app that pip-pins its own
kiro_crew copy cannot shadow the gateway's code. The deps dir joins
the app-copy denylist so install/update never drags a foreign-platform
wheel tree along.

Closes #7878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Packaged install cannot provision an app's requirements.txt: bundled interpreter has no ensurepip, and the failure is not surfaced

3 participants