feat: managed-venv atomic updates — shadow engine + in-app apply with host step-up - #6155
Conversation
0de5a5a to
c761597
Compare
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound RFC-backed shadow-venv design; but a shared security-primitive change rides along undocumented, and no-pruning turns every update into permanent disk growth. Watch
Suggestions
[DESIGN-REVIEWED] f29e38e |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @bolichen97 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The update engine earns its place; its manifest verifier is a third copy of What this change shipsIntent: let a
More items exist; these are the ten a person notices. Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] f29e38e |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've now traced all three candidates against the actual code. Candidate 1 (unredacted traceback on generic Candidate 2 (arm accepts empty channel): Candidate 3 ( No grounded Step 2 finding survives falsification. No findings. [OPUS-REVIEWED] f29e38e Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
c761597 to
d857053
Compare
|
Addressed both converged findings at head d857053: GPT BLOCKING 1 / Design Watch (rmtree without ownership or liveness proof) — fixed by all three of the suggested measures:
GPT BLOCKING 2 (respawn_executable on the event loop) — GPT FINDING (function-local imports) — both Gates re-run: black (new files), isort, flake8, mypy full-tree, 53 engine tests + 469 update/restart subsystem tests green. |
d857053 to
129783d
Compare
129783d to
705d4e6
Compare
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Solid step-up flow, but the armed state evaporates on refresh, and both committed screenshots render an entirely textless UI that contradicts the PR's captions. Watch
Suggestions
[UX-REVIEWED] f29e38e |
705d4e6 to
e77ae69
Compare
b8fd04f to
b4e34f2
Compare
b4e34f2 to
efb515e
Compare
|
Addressed both round-8 findings at head 87babe1: BLOCKING (raw exception logged before redaction) — the redaction now runs BEFORE the log line as well as the progress push: the kiro_crew logger feeds the ring buffer /api/logs streams to the dashboard, so a raw log line was the same exposure as the raw progress push fixed in an earlier round. FINDING (wrong header name) — correct: the middleware's internal-route branch validates X-Internal-Secret; X-Local-Secret is the route-specific mechanism for /api/token/local. The CLI now sends X-Internal-Secret. |
efb515e to
87babe1
Compare
|
Addressed both round-9 findings at head 6e3524a: BLOCKING 1 (stable link as restart executable) — HARDENED rather than reverted, with the reasoning stated because the suggested fix (return sys.executable unconditionally) would remove the feature's core mechanism, not a side effect of it: restarting through the stable link is what makes a promotion take effect at all (RFC update-architecture §3 lists updates.py's re-exec among the four persisted launch paths that MUST resolve through the stable name — re-exec'ing the cached sys.executable is precisely the resurrect-the-old-tree bug §3 exists to prevent). On the threat itself: an actor who can repoint the link is an approved shell running as the gateway's user, and that actor can already rewrite the tree sys.executable lives in — the RFC's security section names local code execution as the gateway's user an accepted gap that "nothing short of a system-installed, root-owned helper" closes; the link adds no privilege the actor lacks. What the fix DOES add: respawn_executable now refuses a stable link whose target resolves OUTSIDE the managed layout's own trees (falls back to sys.executable), so the link is pinned to WHERE an interpreter may live even though it cannot pin WHO wrote it — honestly scoped in the comment. Test pins the repointed-outside fallback. BLOCKING 2 (disk exhaustion tracebacks) — the wheel write and the shadow-directory claim both convert OSError to WheelUpdateError, which the CLI and the approve endpoint already catch and narrate. Test pins the ENOSPC shape. |
|
Addressed both round-10 findings at head c6e6f3f: BLOCKING 1 (interrupted promotion strands the verified tree) — the sentinel now comes off only AFTER promote() succeeds: a crash between verify and promote leaves the marker in place, so the retry's reuse guard still recognizes the tree as ours-and-clearable. The brief post-promote window in which a PROMOTED tree still carries the sentinel is covered by the restored stable-target refusal in build_shadow_venv (checked before the sentinel guard), so it can never be rmtree'd either. A failed unlink after a successful promote reports instead of raising — the update already happened. BLOCKING 2 (publication failures report success) — a repoint_launcher_symlink refusal now surfaces in the progress feed with the consequence stated (new shells keep the previous version until the installer re-runs); the stable link itself is already fail-loud through promote(). cli.sh's stable-link maintenance keeps its non-fatal warning-to-stderr by design (the direct launcher symlink it writes next keeps working without the link) — that half was already surfaced, not swallowed. Also fixed the Backend Tests red this head inherited from round 8's header change: cli_server.py now sets X-Internal-Secret, which put it in scope for the mcp-call-site auth-coverage guard — registered in _SOURCES, with its two structurally-unresolvable sites added to the reviewed set with justifications (the /api/ready liveness probe, and the release-feed fetch whose peer is the CDN, not the gateway). |
|
Addressed both round-11 findings at head 1bc2170: BLOCKING 1 (nonce briefly world-readable under umask 022) — the temp file is now born 0600 (os.open with O_CREAT|O_EXCL, mode 0o600, then fdopen-write), replacing the write-then-restrict sequence whose pre-tighten instant was the exposure; the trust/ parent is created via make_owner_only_dir. No readable moment exists at any point. BLOCKING 2 (prefix matching misclassifies unrelated venvs) — a sibling directory is now POSITIVELY identified before it counts as managed: name convention alone ("crew-" prefix under KIROCREW_VENV=/srv/crew catching /srv/crew-dev) no longer suffices — the candidate must also carry this engine's own artifacts: bin/kirocrew (every completed install ships it) or the build sentinel (every in-flight build). The legacy root and the stable link's target keep identity-based matching. Also pruned the black baseline (a formerly-baselined file this PR touched became clean), per the gate's own instruction. The 3.10-shard-4 reds on the prior head (test_update_provider kill-tree + OMC reap) reproduce as flaky mock-timing on that runner only — both pass locally and on 3.12/Windows; not addressed by code. |
|
Addressed all three round-12 findings at head d0b2733: BLOCKING 1 (agent-writable staging permits wheel substitution) — the whole staging area (manifest workdir + downloaded wheel) moved from TMPDIR into BLOCKING 2 (approval not SEL-audited) — BLOCKING 3 (500 MiB buffered download) — the wheel download is now streamed to disk in 64 KiB chunks with the size cap AND the SHA-256 folded in incrementally; the digest covers exactly the bytes on disk, an over-cap or tampered transfer deletes its partial, and the old buffer-everything read is gone from the wheel path (the 64 KB manifest keeps the bounded in-memory read). Tests rewritten to the streaming seam: chunked round-trip, mismatch-deletes-partial, cap-against-received-bytes, ENOSPC narration, HTTPS pin on both paths. |
|
Addressed both round-13 findings at head d089212: BLOCKING 1 (prefix-matched siblings still admitted by identity) — correct: round-11's positive-identification rule was added to the fallthrough branch while the FIRST loop still enumerated prefix-globbed siblings by identity, so a foreign crew-venv-dev was admitted before the artifacts check could refuse it. Identity trust is now the legacy root ONLY; every versioned sibling — genuinely promoted trees included — must pass the convention-plus-artifacts branch (bin/kirocrew or the build sentinel, which every real install/build carries). The stable link's target is deliberately not an identity root either: the respawn guard uses this predicate to detect a link repointed OUTSIDE the layout, and trusting the target by identity would make that check vacuously true. The prefix-glob helper is deleted outright. New test pins the crew-venv-dev refusal. BLOCKING 2 (critical SEL write on the event loop) — correct: a critical=True SEL write flushes inline on the calling thread by design (fail-closed audit). _audit is now async and offloads via asyncio.to_thread at all four call sites (denied/granted/failed/success). |
|
Rebuttal to the round-14 BLOCKING (stable link as exec pointer), before the override: The demanded fix removes the feature's core mechanism, and the threat it cites is exactly equivalent to the pre-existing baseline. Evidence:
Residual honestly stated in the code comment at the guard: the layout check pins WHERE the interpreter may live, not WHO wrote it. Closing the WHO requires fencing the trees (follow-up worthy, tracked in the RFC's open items), and applies identically to the no-link baseline. |
|
/ai-review override gpt d089212: the demanded revert removes RFC §3's required stable-link restart while the cited threat is exec-pointer-equivalent to the agent-writable sys.executable baseline (crew-venv is not a sensitive path; verified), per the equivalence evidence in the rebuttal comment above. |
Human judgment recorded@bolichen97 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. |
|
Disposition — GPT round 15 (target=gpt head=67624a3556707c0e8c94c25d48e094a98999f63c) FIXED. Finding: Fix (whole-class, not per-site):
Commit-level verified: |
|
Disposition — GPT round 16 (target=gpt head=f1896396fe486d819e02e1e7cfd13c1de31affdb) B1 (updates.py — critical approval audit fails open): FIXED. FINDING (AboutPanel expired-vs-applying in a throttled tab): FIXED. armed:false on the wire covers both consume and TTL lapse; a throttled tab's decremented counter never reaches zero, so the poll misread its own expiry as an approval. The armed state now carries an absolute B2 (sentinel-authorized rmtree): REBUTTED — will not implement.
If this finding returns unchanged next round it meets the operator-authorized override criteria (re-litigation of a rebutted finding whose demanded change breaks the documented design). |
|
Disposition — GPT round 17 (target=gpt head=56a2af157319a4d5b5dd07e1d2060af3ce7f06a8) Both items FIXED. (B2 from round 16 was not re-raised — the rebuttal stands.) BLOCKING (AboutPanel poll effect render loop): FIXED. The round-16 deadline fix added FINDING (manifest refuses optional |
|
Disposition — GPT round 18 (target=gpt head=56a2af157319a4d5b5dd07e1d2060af3ce7f06a8) BLOCKING (agent-executable helper bypasses host-local approval): REBUTTED — false premise, no code change. The chain's load-bearing step — "dashboard bearer … approves Bash running
The rule that fires on the interpreter spellings is explicit that the import is the gate: "an inline interpreter program imports it … the import is the gate and no 'token' word need appear." Any Second, independent fence (demonstrated live this cycle): a command that merely references the nonce path is blocked by the sensitive-credential-path gate before it runs — my own read-only probe that named the path string was refused by that layer. The design's security argument is unchanged and correct: reading the owner-only nonce proves host-filesystem access as the gateway user, which a remote dashboard bearer lacks — AND the one local principal who could read it (the agent) is denied every code path that would. The revert GPT asks for would remove a shipped, RFC-OQ7-required capability to defend against an attack the denied-command + sensitive-path layers already close. If this returns unchanged next round it is a re-litigation of a rebutted finding and meets the operator-authorized override criteria. |
|
Formal adjudication record for the ROUND CONVERGENCE ledger. Rationale lines are quoted so each ruling's coverage is scoped.
|
|
|
|
|
The cli.sh managed-venv install could not apply an update itself: the gateway runs from the very venv an in-place upgrade would overwrite, so kirocrew update re-ran the installer over the live tree and asked the user to restart by hand. Implement RFC update-architecture §3's versioned-trees design for that shape. kirocrew update now builds crew-venv-<version> as a fresh sibling tree (manifest RSA signature verified against the cli.sh-pinned offline trust root, wheel SHA-256 against the signed digest), proves the tree imports the promised version, atomically promotes a crew-venv-current stable symlink (sibling link + os.replace, never ln -sfn), repoints the ~/.local/bin launcher, and prunes stale trees while never touching the stable target, the running tree, or the legacy directory. Every failure before promotion leaves the install untouched; the legacy fixed venv is never moved, per the first-migration protocol. Gateway restarts resolve their interpreter through the stable link (respawn_executable), so a restart after promotion picks up the new tree instead of re-exec'ing the superseded sys.executable. cli.sh repoints the stable link at the legacy tree after its own installs, so the link always names the last-installed version whichever writer ran. pipx installs keep the installer re-run; Windows is unaffected. The in-app Apply button stays deliberately out: RFC Open Question 7 requires a host-local step-up before a dashboard session may trigger a code install. That lands separately.
|
Disposition — GPT round-23 (head b7a5778 -> fixed in f29e38e)
Prior rulings carried forward unchanged: B2 sentinel-rmtree (capability equivalence), round-18 python-c-nonce (deny-layer tested), round-19 cli.sh EEXIST pre-unlink, round-20 boot-path lazy import, round-21 launcher-handoff recovery, round-22 url-userinfo regex. |
|
/ai-review override gpt f29e38e: Composite re-litigation — the demanded "revert stable-link execution" is verbatim the round-14 finding already overridden on exec-pointer equivalence, the "agent plants tree + repoints link" mechanism is the B2 capability-equivalence terrain already rebutted (ledger 5436047633), and the recovery branch it attacks is GPT's own round-21 requested fix. Evidence recap (all previously established on this PR's ledger):
|
Human judgment recorded@bolichen97 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. |
Summary
The
cli.shmanaged-venv install (the README's headline install) has never been able to apply an update itself: the gateway runs from the very venv an in-place upgrade would overwrite. This PR ships the full RFC-update-architecture Phase 2 path for that shape — the shadow-venv engine, the CLI flow, AND the dashboard in-app Apply with its Open Question 7 host-local step-up.1. Shadow-venv atomic update engine (
platform/wheel_engine.py)Implements §3's versioned-trees invariants and first-migration protocol:
kirocrew updatebuildscrew-venv-<version>as a fresh sibling tree while the current install keeps serving; the RSA-signed manifest is verified against the same offline trust rootcli.shpins (a drift test asserts the constants stay byte-identical), the wheel's SHA-256 against the signed digest, and the tree must import the promised version before anything is promoted.crew-venv-currentstable symlink replaced atomically (sibling link +os.replace, neverln -sfn). The legacycrew-venvdirectory is never renamed, moved, or converted — it stays a functional fallback.respawn_executable, off-loop), so a restart after promotion picks up the new tree instead of re-exec'ing the supersededsys.executable.cli.shrepoints the stable link after its own installs, so the link always names the last-installed version whichever writer ran.pipx install --forceunder a running gateway is the exact hazard this avoids). Windows unaffected.2. In-app Apply with the OQ7 step-up (
platform/update_stepup.py)RFC OQ7 (human-review-resolved) rules that a dashboard session — a transferable bearer under tunnels (#1762) — is not sufficient authority to install code. The in-app flow therefore splits authority:
POST /api/update/armrecords the pending request and writes a single-use approval nonce (TTL 10 min) owner-only into the data home. The response andGET /api/update/armcarry the request id and the approve command — never the nonce.kirocrew update approvereads the nonce file — filesystem access as the gateway's user is the identity being proven — and presents it back (POST /api/update/approve, loopback-gated, unix-socket/SO_PEERCRED preferred so token-auth installs need no dashboard token). The gateway then runs the shadow apply itself with progress on the existing update-overlay SSE, and restarts into the new version.3. Dashboard UI (
AboutPanel.tsx)The managed-venv branch gains an Update button gated on the new
update_can_armstatus field (probed server-side —managed_byalone also covers bare source installs whose arm would 409). Clicking it arms and swaps to the armed state: copyablekirocrew update approve, countdown, and a liveness poll that notices approval/expiry from the terminal side. The manual installer command stays reachable behind a fold. Five new i18n keys in all 12 locales + regenerated en-XA.UI states:
Captured with Playwright against a vite harness; the vitest suite pins the same states via DOM assertions — including that nothing nonce-shaped ever appears in the DOM.
Deliberately out of scope (per the RFC)
cli.sh's venv branch does — parity, not regression).~/.local/bin/kirocrew, which the engine repoints).Tested
cli.sh, manifest rejection matrix, real RSA sign/verify round-trip, promotion atomicity, promoted-tree rebuild refusal, concurrent-update lock, layout/detection, respawn fallback, launcher-repoint refusals, shadow-build guards, orchestration ordering.update_can_arm, arm swaps to armed state with no nonce-shaped secret in the DOM, arm refusal surfaces inline, manual command reachable.Bundle budget note:
CHUNK_BUDGETS.t(i18n runtime + English catalog) raised 700→770 KB. The chunk crossed the old ceiling by ~0.6 KB from catalog-string growth — this PR's 9 in-app-update keys plus main's mandatory-update floor strings landing in the same window — the growth class the budget's own comment calls expected; no new library reached the runtime module.