fix: provision app requirements via pip --target, surface pip failures (#7878) - #7901
fix: provision app requirements via pip --target, surface pip failures (#7878)#7901chenmingwei23 wants to merge 1 commit into
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] 09111ad |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/apps/backend.py:1115 -- function-local False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS A 170-line exec-capable launch shim ( Not justified as shipped
What this change shipsInventory (10 items) — 6 justifiedIntent: make apps that declare a
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 09111ad |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
0f07927 to
8e0b908
Compare
8e0b908 to
a77000e
Compare
a77000e to
3ec9bb9
Compare
b4987a3 to
f35246d
Compare
4003f14 to
792560c
Compare
Escalation: round 18 trips the non-convergence tripwires (maintainer decision required)The automated pipeline has fixed or dispositioned every finding through 17 Round-18 findings and assessment:
Structural options for the maintainer: A. Launch-shim approach: spawn provisioned backends through a tiny stub B. Documented-refusal approach: extend the existing editable refusal to any C. Accept GPT's revert prescription and abandon --target provisioning -- Under any option, finding 1's link-aware purge and finding 3's redaction Current state on head 792560c: every lane Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c] |
792560c to
3ee8e39
Compare
|
Round-18 resolution on head 3ee8e39
All lanes re-roll on this head. Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c] |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
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 Full round history:
Open round-21 findings (both look real, both small):
Everything else on head 2da8c9b is green What the pattern says: the mechanism (stamp inference + argv rewriting) has The branch and worktree (/tmp/kc-fix-7878) are intact. The two fixes above Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c] |
Maintainer handoff (final automated round)On operator instruction, the two round-21 findings are applied as this
Both pinned by tests; all local gates green (482 tests, black/flake8/ The automated pipeline is now DONE on this PR and will not push again. Branch and worktree are intact. Summary of what this PR now ships is in Kiro Crew Auto-Pipeline [operator: chenmingwei23#de330d0c] |
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.
Windows note: items 1-3 keep the transaction's existing Windows floor (no dir_fd there): New regression test: 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. |
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 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 (
Regression tests added: a sensitive-path command is refused by all three readers BEFORE any 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. |
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)
What LEAVES (tracked in #9396, to be REDONE - not ported - in a follow-up PR)
WhyReview 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 behaviorAfter 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. |
|
@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. 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: Rebase. The audited base was 712 commits behind main. Expect a one-hunk conflict with #6599 and #8778 in 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. |
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. |
|
@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):
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 One scope question for you. The single uninstall-adjacent piece still in this branch is the generated-artifact purge in 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. |
|
/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 |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
@bolichen97 Closing out the one open scope question from my earlier reply: the generated-artifact purge in The reasoning is ownership, not boundary preference: the purge removes exactly the 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
Problem / Motivation
On a packaged install, provisioning an app's
requirements.txtcan never succeed: the bundled interpreter ships pip but noensurepip, so the per-apppython -m venvdies right after creating the directory skeleton. Neither half of the failure reaches the user -- venv creation collapses into alogger.warning, and thepip installcall passes nocheckat all, so a non-zero pip exit is discarded without even a log line. The backend then spawns without its dependencies and dies on aModuleNotFoundErrorthat points at the app, not at provisioning. Worse, the half-built.venvskeleton is runnable enough thatresolve_app_pythonprefers 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.txtalongside 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, sopip install --targetsidesteps the bootstrap entirely.apps/backend.py-- the venv+pip pair becomes onesys.executable -m pip install --target <app>/.kirocrew-deps -r requirements.txtwithcheck=True. The deps dir is prepended to the child'sPYTHONPATH(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 SELdeps_provision_failedevent, 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.pip --targetcannot answer "already satisfied", so a digest ofrequirements.txtplus 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_pythonreturnssys.executableunconditionally (those wheels are ABI-bound to it). Without provisioned deps it prefers an app-shipped.venvonly on a probe that positively confirms the venv is usable -- its interpreter starts, reports asys.prefixinside the venv, and matches the gateway's Python minor version. Apyvenv.cfgversion check was not enough in either direction (a failedvenvskeleton passes it; a working cross-minor venv fails it), so the probe is positive evidence, not a heuristic.venv_provided_commandmirrors that precedence and probes<deps dir>/bin(Scriptson Windows) where--targetplaces console scripts, so bare console-script commands in app manifests stay resolvable.kiro_crewpackage code is spawned from a writable app dir; it never provisions arequirements.txtfound there and never gets.kirocrew-depson itsPYTHONPATH, so agent-authored wheels cannot load ahead of the trusted module.apps/bridges.py-- stdio MCP server registration exposes the deps dir throughPYTHONPATHthe same way the backend spawn env does; a--targetinstall 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/app-kit/manifest-reference.md(stdio command resolution) anddocs/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 singlepip --targetinto staging with no-m venvanywhere; the pip argv issys.executable -m pip(never a bare or venv-relative interpreter);check=Trueis 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'sPYTHONPATH; a failure logs at ERROR and writes the header into the backend log; the deps dir lands first on the child'sPYTHONPATH, 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-minorpyvenv.cfg) is rejected by the probe; a venv with no interpreter falls back tosys.executable; provisioned deps pinsys.executable; deps-dir console scripts resolve, with a usable venv winning on a name collision only when no deps dir was provisioned.TestDependencyInstalladds a module-builtin case asserting no provisioning and no deps-dirPYTHONPATHinjection.test_app_bridges.py::TestStdioDepsDirExposure-- stdio registrations get the deps dir prepended toPYTHONPATH(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-depswhile 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 --targetplaces console scripts in<target>/binwith 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 ensurepipfailing 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, nocheck=, 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)
deps_boot(a stdlib-only shim thatsite.addsitedirs the deps dir before dispatching the real target), because PYTHONPATH never processes.pthfiles - 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.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_appvalidates the app name before any metadata read (_check_path_safety), unchanged from main.