Skip to content

feat: make the managed-python choice sticky across installer runs - #4646

Open
CrysisDeu wants to merge 1 commit into
mainfrom
feat/sticky-managed-python
Open

feat: make the managed-python choice sticky across installer runs#4646
CrysisDeu wants to merge 1 commit into
mainfrom
feat/sticky-managed-python

Conversation

@CrysisDeu

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #4594. Two changes:

1. The managed-python choice is now sticky. A --managed-python install records its interpreter mode in the data home (python-mode, next to channel). A later installer run with no explicit flag or KIROCREW_MANAGED_PYTHON value reuses it — most importantly the re-run that kirocrew update performs, which composes only --channel (update_layout.wheel_update_command). Without the marker, every update silently flipped a managed install back onto whatever system interpreter it found, discarding the user's choice. A new --system-python flag opts back out explicitly and re-records the mode; an explicit flag or env value always outranks the marker (three-state resolution: flag/env > marker > system default).

2. README documents the experimental install command. The One-line install section gains the --managed-python one-liner with the sticky semantics spelled out; docs/guides/install.md updated in the same commit.

Tests

  • test_cli_reuses_a_recorded_managed_python_choice: with a managed marker on disk and a perfectly usable system python3.12 on PATH, a flag-less run must reach for the pinned uv tarball (curl marker) and print the reuse notice.
  • test_cli_system_python_flag_overrides_the_recorded_choice: --system-python with the marker present must use the system interpreter and never touch uv.
  • Installer suite 12/12 green; isort/flake8/mypy/docs-lint/brand gate clean; full backend suite green (11 failures verified pre-existing on clean origin/main on this host: test_artifact_source/test_artifacts_handlers/test_xdist_host_budget/test_history_coverage/test_prepare_pr_prove).

Manual verification

Real E2E on an isolated instance (fresh HOME/KIROCREW_HOME, aarch64 Linux): --managed-python install writes python-mode=managed; a flag-less re-run prints "Reusing the recorded managed-python choice", rebuilds the venv against the PBS interpreter (pyvenv.cfg home verified), and kirocrew --version runs.

no linked issue: gap found while answering "does an upgrade need the flag again?" right after #4594 merged.

Why no screenshot: installer shell script, docs, and backend test only; nothing renders in the browser.

@CrysisDeu
CrysisDeu requested a review from a team as a code owner August 20, 2026 06:54
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real regression path (updates silently flipping a managed install), fixed at the right layer — the installer itself — so all flag-less re-runs are covered, not just kirocrew update.

Suggestions

  • Installs made under feat: provision Python via pinned uv in cli.sh instead of the distro ladder #4594 before this ships have no marker, so their first update still flips them to system — and then records system, making the wrong choice sticky. A one-time back-fill (treat a pyvenv.cfg pointing into the managed interpreter dir as managed when no marker exists) would close the exact harm this PR names for that cohort.
  • The symlink/FIFO/directory hardening of the marker read/write (including the pre-existing channel write) is a real security change the PR description never mentions — name it there so reviewers of the agent-boundary surface see it.

[DESIGN-REVIEWED] c293131

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c293131

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

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

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've read the instruction file, the candidate list, and the full diff. Let me verify the one candidate against the actual code behavior.

The single candidate (CANDIDATE 1) claims head -c 16 is non-POSIX and would silently drop stickiness on hosts whose head lacks -c.

Testing (a) — a concrete input that occurs in practice: The script targets macOS and Linux. BSD head (macOS), GNU coreutils head (Linux), and busybox head all implement -c. The candidate's own reasoning concedes it "could not confirm any supported target lacks it." So the required condition — a head implementation without -c on a supported platform — does not occur in practice. (a) resolves to "if a host were to…", which the falsification bar forbids. The candidate is speculative surface, not a grounded defect. Dropped.

I also independently checked the neighboring changed logic for grounded defects:

  • The set -e interaction with [ -L … ] && rm -f … and [ -d … ] && err … — both are standalone AND-OR lists; a false left operand short-circuits and is exempt from set -e, so no spurious exit. Correct.
  • Command substitution $(head -c 16 …) strips the trailing newline, so "managed\n" compares equal to "managed". Correct.
  • Symlink-to-file, symlink-to-directory, and directory-target cases in _write_marker are each handled (rm the link first, refuse a real directory, atomic mv of an O_EXCL temp). No missed case.
  • Explicit env/flag override paths (MANAGED_PYTHON empty vs 0 vs 1) all resolve correctly through [ -n "$MANAGED_PYTHON" ] || MANAGED_PYTHON=0.

Nothing grounds to the (a)/(b)/(c) bar at 80+.

No findings.

[OPUS-REVIEWED] c293131

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

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

@CrysisDeu
CrysisDeu force-pushed the feat/sticky-managed-python branch from 7d1a1f8 to 4ea979a Compare August 20, 2026 07:23
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • cli.sh:569 — interpreter marker follows agent-planted symlinks: fixed in 4ea979a via the suggested mktemp+atomic-rename shape (_write_marker): mktemp creates a fresh regular file (O_EXCL, never a symlink) and rename() REPLACES a symlink at the destination instead of following it. Applied to the adjacent channel write too — it had the identical defect (sibling-branch completeness). New regression test plants a symlink at the marker path pointing at a victim file, runs the exact write shape, and asserts the victim is untouched and the marker is a regular file.

@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 Aug 20, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/sticky-managed-python branch from 4ea979a to 76ccde7 Compare August 20, 2026 07:47
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • cli.sh:574 — directory marker targets corrupt persisted state: fixed in 76ccde7. A real directory at the marker path is refused loudly (err + guidance) before any mv. While fixing, also covered the sibling hole the finding implies: a planted symlink-to-DIRECTORY makes mv move the temp file INSIDE the target rather than replacing the link — the guard now removes a pre-existing symlink (whatever it points at) before the rename. Three regression tests: symlink-to-file (victim untouched), symlink-to-directory (nothing lands inside the target), plain directory (loud refusal, temp file not moved in).
  • test_installer_distro.py:442 — filesystem-mutating child inherits the repository CWD: fixed in 76ccde7 — the marker-write subprocess (now a shared helper) passes cwd=tmp_path.

@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 20, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/sticky-managed-python branch from 76ccde7 to 9da8716 Compare August 20, 2026 08:16
@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: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Aug 20, 2026
A --managed-python install records its interpreter mode in the data home
(python-mode, next to channel). A later installer run with no explicit
flag or KIROCREW_MANAGED_PYTHON value reuses it -- most importantly the
re-run that 'kirocrew update' performs, which passes only --channel.
Without the marker, every update silently flipped a managed install back
onto whatever system interpreter it found, discarding the user's choice.

--system-python (new) opts back out explicitly and re-records the mode;
an explicit flag or env value always outranks the marker.

README gains the experimental one-liner for --managed-python installs,
and the install guide documents the sticky semantics.

Verified end-to-end on an isolated instance: install with
--managed-python (marker written as 'managed'), re-run with no flag --
the run reuses the recorded choice and the venv stays on the PBS
interpreter. Hermetic tests cover the marker read and the
--system-python override.
@CrysisDeu
CrysisDeu force-pushed the feat/sticky-managed-python branch from 9da8716 to c293131 Compare August 20, 2026 08:42
@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 Aug 20, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • cli.sh:313 — untrusted marker reads can wedge installer updates: fixed in c293131 exactly as suggested: the read now requires a plain regular file (-f && ! -L) and is bounded (head -c 16) instead of an unbounded cat. Two regression tests: a planted symlink (spoofing managed) is ignored and the run stays on the system interpreter; a planted FIFO is never opened (the test would hang at the harness timeout if the -f guard were lost).

Marker-persistence branch table (rounds 2–4 all landed in this span; recording completeness so the next round can check against it):

Surface Case Guard Test
write symlink → file rm link, then mktemp+rename test_cli_marker_write_replaces_a_planted_symlink
write symlink → directory rm link before rename test_cli_marker_write_survives_a_symlink_to_a_directory
write real directory loud err refusal test_cli_marker_write_refuses_a_directory_target
write absent / regular file atomic mktemp+rename sticky E2E
read symlink (any target) ! -L → ignored test_cli_marker_read_ignores_a_planted_symlink
read FIFO / non-regular -f → never opened test_cli_marker_read_never_opens_a_fifo
read oversized / corrupt head -c 16 bound; mismatch → ignored covered by bound + equality
read regular managed honored test_cli_reuses_a_recorded_managed_python_choice

Note: Backend Tests (Windows) (3) on the previous head was the known Windows Event loop is closed cascade (test_mcp_sync_agent/test_md_notebook, zero Python source changes in this diff); this push's fresh round re-runs it.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant