Skip to content

feat(update): gate venv removal on install provenance, not a name diff (#15067) - #15572

Merged
mrveiss merged 2 commits into
Dev_new_guifrom
issue-15063-venv-reconcile
Sep 3, 2026
Merged

mrveiss merged 2 commits into
Dev_new_guifrom
issue-15063-venv-reconcile

Conversation

@mrveiss

@mrveiss mrveiss commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Thinking Path

#15063's reconciler — the thing that makes the deployed venv no longer
append-only — already exists on base and is untouched by this PR. What it
carried was a KNOWN LIMITATION in its own docstring, which is what #15067 is:

removal decided by diffing installed names against the declared set cannot
tell a package this tool installed from one an operator reinstalled under
a name the tool's history once declared.

That is a data-loss-shaped failure on someone else's machine, so the obvious fix
to #15063 is the dangerous one. This PR closes that hole.

Single-issue rationale: dispatched as a #15063 + #15067 batch; #15063 turned out
already implemented, so only #15067 has a diff. It closes #15067 alone rather
than claiming a closure with no change behind it.

What Changed

Provenance is a marker written into each package's own dist-info directory
after a reconcile run confirms it installed.

Why not the lock file — the obvious place: a lock records that a name was
declared, not that the on-disk install is still the one this tool put there.
pip fully replaces a distribution's dist-info on install or uninstall, so
marker-presence is evidence nothing has touched that install since the tool last
confirmed it, and absence is evidence something has. That is precisely what
pip's own INSTALLER field cannot tell you, which the issue calls out.

split_by_provenance splits removal candidates into verified and unverified;
only verified are removed.

The first-run case, decided rather than emergent

This is the part that mattered most, because every deployed host today has a
venv with no markers
— including packages this tool genuinely did install:

Treat "no marker" as Consequence
ours first run destroys operator packages — the exact #15067 failure
never ours #15063 stays dead forever on every existing host

Neither is the default. Unverified candidates are refused and reported in the
job's own steps, and removing them takes an explicit env opt-in
(AUTOBOT_VENV_RECONCILE_ALLOW_UNVERIFIED_REMOVAL, default false) that an
operator can review the reported list against, set for one run, and unset. That
is the reversible first form the change needed.

Meanwhile every still-declared package is re-marked on every run, so marker
coverage — and therefore #15063's automatic behaviour — grows on its own with no
migration step and no flag day.

Verification

$ python3 -m pytest autobot-slm-backend/tests/test_venv_reconcile.py \
    autobot-slm-backend/tests/test_venv_provenance.py -q
38 passed in 3.83s

$ python3 scripts/check_python_file_size.py --audit-ceilings
5050 files scanned, 499 grandfathered, all live and at size.

Three behaviours, three tests, each mutation-proven:

Behaviour Mutation Red
a dropped package is removed invert verified/unverified assert () == ('pkg-orphan',)
an operator's reinstall is not removed never skip assert () == ('pkg-stale-collide',)
unknown provenance is refused has_tool_provenance → True 3 tests red

The orphan-removal test stamps a real marker and runs against a real throwaway
venv, so the "IS removed" path is exercised rather than mocked.

A test was inverted, not added.
test_reconcile_removes_a_name_an_operator_reinstalled_after_it_was_declared
became ..._survives_.... That inversion is the issue — the old assertion
encoded the defect, so it had to change rather than gain a sibling.

New code lives in a new top-level module rather than under api/, because
api/__init__.py imports every router and would break the standalone
test-loading pattern test_venv_reconcile.py already documents.

Needs host evidence — do not close #15063 on this

#15067 is closable on CI. #15063 is not, and this PR does not claim it:
confirming that the first reconcile on a real host with a pre-existing lock and
zero markers reports its candidate list rather than silently dropping it, and
that the opt-in works end to end through the real update job, requires a
deployed host.

That caution is not theoretical today — a live self-update an hour ago wrote new
code at 16:18:15 while the process performing it had been up since 16:15:14, so
the running code was three minutes older than what it deployed. Exactly the
#15323 trap.

Model Used

Claude Opus 5

Closes #15067
Refs #15063

#15067)

#15063's reconciler already exists and is untouched here. What it carried was a
KNOWN LIMITATION in its own docstring: removal decided by diffing installed
names against the declared set cannot tell a package this tool installed from
one an operator reinstalled under a name the tool's history once declared. That
is the operator-package destruction #15067 exists to prevent.

Provenance is a marker written into each package's own dist-info directory after
a reconcile confirms it installed. A lock file cannot serve here: it records that
a name was DECLARED, not that the on-disk install is still the one this tool put
there. pip fully replaces a distribution's dist-info on install or uninstall, so
marker-presence is evidence nothing has touched that install since, and absence
is evidence something has -- which is what pip's own INSTALLER field cannot say.

The first-run case is decided rather than left to fall out. Every deployed host
has a venv with no markers, including packages this tool did install. Treating
absence as "ours" destroys operator packages on the first run this ships;
treating it as "never ours" leaves #15063 dead forever on every existing host.
Neither is the default: unverified candidates are refused and REPORTED in the
job's own steps, and removing them takes an explicit env opt-in an operator can
review the reported list against and then unset. Meanwhile every still-declared
package is re-marked each run, so coverage grows without a migration step.

The test that used to assert an operator's reinstall IS removed now asserts it
SURVIVES -- the inversion is the point of the issue, so it is the test that had
to change, not be added alongside.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

Applied Black, isort, and autoflake to match code-quality checks.
Triggered by workflow auto-fix-formatting.yml.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant