Repository navigation
feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) - #17725
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds workspace lease acquisition, release, and reclamation. It integrates leases with heartbeat scheduling and adds a scheduled process for reclaimed workspaces and approved disposal proposals. Worktree removal now resolves the owning Git repository first. ChangesWorkspace lease lifecycle
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Concurrent workspace acquisition and delayed lease reclamation remain material risks. Resolve or explicitly accept them before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Human approval, fresh Git checks, and non-forced removal substantially limit risk. However, a workspace can be leased again between the ownership check and removal, and successful removal can precede durable recording of its outcome. Worker permissions and filesystem containment also remain unresolved. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Review — one real defect (the model still declares the constraint the migration drops), one question that is the owner's, and both of the named checks verified.
Scope, stated so it is not mistaken for more: +1403 lines. I reviewed the migration, workspace_disposal.py, acquire_lease, the model, and the beat wiring — the two areas asked about plus what they depend on. I did not line-review the sweep, the run-context/output modules or the ~450 lines of tests beyond reading their names. Nothing was run.
The defect: the ORM still declares unique=True on path
The migration drops uq_llc_workspace_leases_path and replaces it with a partial index. The model is not in this PR's diff, and at origin/main it still says:
autobot-backend/llc/models/workspace_lease.py:48
path: Mapped[str] = mapped_column(sa.Text(), nullable=False, unique=True)So after this lands the declared schema and the migrated schema disagree: a database built from the models carries the table-wide UNIQUE and reproduces the exact bug this migration exists to remove, while a migrated one does not. Nothing catches it — I looked for a guard comparing declared constraints to migrated ones and found only repo_tests/orphaned_database_migrations_test.py, which is about orphaned revisions, and no test in the tree calls get_unique_constraints on this table. Silent divergence, in the same PR that fixes the thing it diverges about.
Drop unique=True from the column and carry the partial index in __table_args__, in this PR.
The predicate is weaker than is_live() — correct today, on an implicit coupling
The migration says it carries "the intent the comment always stated". The comment's intent is defined in code three lines below it:
autobot-backend/llc/models/workspace_lease.py:71-75
def is_live(self, now=None) -> bool:
"""Held and not yet expired. A released lease is never live, whatever its expiry."""
if self.released_at is not None:
return False
return self.expires_at > (now or ...)Live is released_at IS NULL and expires_at > now. The index is released_at IS NULL alone — and it cannot be otherwise, because a partial index cannot call now(). The service layer agrees with is_live() everywhere (live_lease_select:78, live_lease_count:111, lease_for_path:129), so an expired-but-unreleased row is invisible to the application and still occupies the index slot.
I traced whether that bites and it does not: acquire_lease calls await reclaim_expired(session, now=moment) as its first statement, so by the INSERT no expired-unreleased row for the path survives, and test_acquire_reclaims_expired_leases_before_counting_capacity pins that ordering. One non-test writer constructs a lease row, so the coupling has exactly one call site.
So: not a defect, an unstated invariant. The index is correct only while every writer reclaims before inserting. Say that in the migration docstring instead of claiming equivalence with the comment — the next writer is the one who will not know, and the failure it produces is an IntegrityError on the path the fleet most wants.
Both named checks: one verified, one is the finding above
_git scrubs the environment — verified. workspace_disposal.py:80 passes env=scrubbed_git_env() (autobot_shared.paths), with the reason stated at :69-71: GIT_DIR would point the command at whatever repository the environment names, with a zero exit code, so a LANDED verdict could be about a different repository entirely (#15783). That is the right instrument and the right explanation.
The disposal discipline around it is the strongest part of the PR: work_landed refuses on a dirty tree and on commits not on a remote, a git failure returns UNKNOWN and still refuses, and dispose_workspace uses no --force — "git refuses a worktree it considers unsafe to remove, and that refusal is a finding rather than an obstacle." That is this repository's own rule implemented rather than restated, and test_a_git_failure_is_unknown_and_still_refuses and test_nothing_is_removed_when_the_check_refuses pin both directions.
The question I cannot answer and should not decide
celery_app.py schedules run_workspace_lease_sweep hourly at minute=50, and that sweep can reach dispose_workspace, which deletes a worktree from disk. The standing rule is that no agent cleans up data on its own — deletion is proposed, a human approves it through the always-available review queue, it is never auto-approved, and it leaves a durable paper trail (#17038).
The case for exemption is real and this PR makes it well: landedness is proven before removal, nothing is forced, and a refusal is recorded. But two things keep it from being obviously exempt, and both are the owner's call rather than a reviewer's:
- it is automatic and scheduled, which is the shape the rule was written about;
- the paper trail is
logger.infoin a celery worker, not a durable record. Even under the exemption reading, #17038 asks for a trail that outlives a log buffer.
Settle this before merge. Deleting a worktree is not reversible, and an hourly job is the wrong place to discover a rule disagreement.
CI
No verdict — the suite was cancelled at 18:13 Riga. A migration, a new celery schedule and a filesystem deleter is the combination that least deserves to merge on an uncancelled-but-unrun suite. Re-run.
Verdict: fix the model declaration in this PR, settle the #17038 question, re-run CI. The rest I would land — and the disposal module in particular is the shape other cleanup code here should copy.
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Owner ruling 2026-09-28: the hourly deleter goes through the approval gateAsked and answered directly by the repository owner, relayed by the coordinating session. Recorded here so the ruling has the paper trail its own subject requires. The hourly celery-beat job may find expired leases and prove landedness — but it proposes disposals to the approval queue and stops. A human approves each batch. Nothing is removed unattended. The approval record is the durable trail; a This is #17038 applied as written — no agent cleans up data or credentials on its own; it proposes, a human approves through the approval gates, never auto-approved, always a durable paper trail — with no exemption carved for proven-landed agent workspaces. The exemption case was put to the owner and declined. The landedness-proof code stays as the proposal's evidence. |
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Second look at ab928a22de — all three findings are fixed, both of the hard questions check out, and there is one new blocker: the sweep stops disposing after twenty proposals, silently.
Verified at ab928a22de rather than taken from the report. Nothing run.
Blocker: .limit() is applied before the executed-ness filter
# workspace_lease_sweep.py:93-105
select(Approval)
.where(Approval.approval_type == …DESTRUCTIVE_ACTION.value,
Approval.status == …APPROVED.value,
Approval.requested_by_agent == SWEEP_REQUESTER)
.limit(MAX_EXECUTIONS_PER_SWEEP) # 20, in SQL
…
return [a for a in result.scalars().all() if _is_sweep_proposal(a)] # executed-ness, in PYTHON_is_sweep_proposal excludes anything carrying executed_at, and nothing ever leaves the APPROVED set — _execute_approved writes context and never touches approval.status. So executed proposals accumulate as APPROVED rows that still match every SQL predicate.
Once twenty of them exist, the LIMIT 20 is satisfied entirely by executed rows, the Python filter drops all twenty, and _approved_proposals returns [] on every subsequent sweep. No approved disposal is ever executed again. There is also no ORDER BY, so which twenty come back is unspecified — the failure can arrive earlier and intermittently rather than at exactly the twentieth.
And it is invisible: the sweep logs disposed 0, refused 0 and reads as a quiet hour. This is the one place in the PR where nothing to do and cannot see the work render identically.
The minimal fix keeps the bound doing its stated job. The constant's own comment says it exists so "a backlog of approvals cannot turn one beat tick into an unbounded run of git operations" — that is a bound on disposals, not on rows selected. So filter first and slice after:
rows = (await session.execute(select(Approval).where(…).order_by(Approval.created_at))).scalars().all()
return [a for a in rows if _is_sweep_proposal(a)][:MAX_EXECUTIONS_PER_SWEEP]Pushing executed_at IS NULL into the SQL (a JSON predicate on context) is the tighter version and equally fine. What I would not do is add an EXECUTED status — the vocabulary is PENDING/APPROVED/REJECTED/REVISION_REQUESTED/WITHDRAWN/EXPIRED (models/approval.py:29-34), and inventing a seventh value to fix a LIMIT would change the approval system for every other gate.
A test for it is cheap and belongs in this PR: twenty-one approved proposals, twenty carrying executed_at, assert the twenty-first is the one acted on.
Both of the questions asked
The refusal is written back — verified, and it persists. Every entry appends to outcomes regardless of verdict, and the record then carries executed_at plus execution_outcomes with path, verdict and detail per path (:112-127). _async_sweep commits at :178. A refusal at execution lands on the same record as the approval and its evidence, which is what #17038 asks for. Re-proof itself is real: dispose_workspace calls work_landed as its first statement, so an approval authorises a removal and does not vouch for the evidence.
The predicate is literally the same expression, in both places:
llc/models/workspace_lease.py:60 postgresql_where=sa.text("released_at IS NULL")
migrations/…_097_…py:85 postgresql_where=sa.text("released_at IS NULL")
Same index name, same single column, same unique=True, and unique=True is gone from the path column (:72). Two copies of one literal rather than one shared constant — which brings me to the test that is supposed to keep them in step.
The model-vs-migration test does not read the migration
test_the_model_declares_the_same_partial_index_the_migration_creates asserts the model against literals written in the test:
index = indexes.get("uq_llc_workspace_leases_live_path")
assert index.unique is True
assert [c.name for c in index.columns] == ["path"]
predicate = str(index.dialect_options["postgresql"]["where"])
assert "released_at IS NULL" in predicateNothing here opens 20260928_097_…py. Its docstring says "Name and predicate both, because either one differing is a silent divergence" — and the comparison is model-versus-test, so if 097's predicate or index name is edited, this test still passes and the divergence it exists to catch is back. There are now three copies of the truth and the test pins two of them to itself.
Also assert "released_at IS NULL" in predicate is a substring check, so a model predicate of released_at IS NULL AND expires_at > now() would pass — a predicate that is both a divergence from 097 and impossible as a partial index.
repo_tests/knowledge_upload_accept_matches_server_17528_test.py is the shape to copy: it reads both sides by concrete literal and compares them. Reading 097's source for its _NEW_INDEX and postgresql_where and comparing to the model's is a handful of lines and makes the claim true.
I am flagging this plainly because it is the same shape as the finding it was written to fix, and as #17729's name-versus-identity assertion: a check whose name states a relationship and whose mechanism examines one side of it.
And the third test is a tautology, though a useful comment
test_the_partial_predicate_is_weaker_than_is_live_and_the_gap_is_the_reclaim constructs a lease and asserts is_live(NOW) is False and released_at is None. Both are true of the object by construction; no index and no database are involved. Its docstring says "if anyone later removes the reclaim, the reason this test exists is in front of them" — the prose will be, and nothing will fail.
What actually fails if the reclaim is removed is test_acquire_reclaims_expired_leases_before_counting_capacity. That is the real pin, and this docstring should point at it instead of implying its own assertions do the work. Worth keeping the test — as documentation it earns its place — with an honest docstring.
Everything else I checked and found right
test_the_model_declares_no_table_wide_unique_on_pathreads__table__.c.path.uniqueand does fail on the old model.- The invariant is now stated in both the model's
__table_args__comment and 097's docstring: weaker thanis_live, cannot be otherwise, permits a second lease on an expired-but-unreleased path, closed by convention rather than schema, with the future-writer failure mode named. requested_by=SWEEP_REQUESTERis the plain string, with the reason recorded — no minted UUID in an audit record.proposed_companyfiles a mixed batch under no company and reaches the platform queue, rather than guessing one company's reviewer.- The sweep logs every count including the zeros, "a sweep that found nothing and a sweep that did not run must not look the same".
One gap for a follow-up, not a blocker
candidates is built only from leases reclaim_expired returned in this run, so a path is proposed exactly once, at reclaim. Combined with executed_at retiring an approval whatever the outcome, a workspace that fails work_landed at proposal time (the continue at :137) or refuses at execution is never proposed again. It becomes precisely the debris the sweep exists to remove, with a logger.info and no record.
That is a gap rather than a regression — before this PR nothing disposed of anything — and it is the right side to err on. But the design reads as complete and is not, so it wants an issue rather than silence.
Verdict: fix the LIMIT ordering in this PR and I would land it. The test-reads-both-sides change and the third docstring are small enough to ride along; the never-re-proposed gap is a follow-up.
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Third look at f0f1a0d9d9 — the blocker is fixed, the schema guard is now a real guard, and the instrument was fixed too. I would land this.
Verified at f0f1a0d9d9; nothing run.
The LIMIT fix, and the docstring is the better half of it
No LIMIT, order_by(Approval.decided_at), pending[:MAX_EXECUTIONS_PER_SWEEP] after the filter. The docstring states the defect, why the bound belongs after the filter, and the sentence that makes it stick — "cannot see the work and there is no work would be identical, in the sweep written to stop exactly that." Declining the JSONB executed_at IS NULL variant on the grounds that an unexercised predicate silently matching nothing is this finding's own failure mode is the right call, and the right reason.
The narrowness question: narrow in predicate, unbounded in time
Asked whether the fetched set stays safe over months. It is safe now and it is unbounded in principle, and the distinction matters because the growth has no ceiling rather than a high one.
The set is DESTRUCTIVE_ACTION + APPROVED + requested_by_agent == SWEEP_REQUESTER. One row per sweep run that proposes at least one landed workspace, hourly: an upper bound near 24/day, far fewer in practice since it fires only when a lease expires and its work is already on a remote. But nothing ever leaves the set — that is the docstring's own premise for having no LIMIT. Executed rows keep status = APPROVED for ever, each carrying a context with the workspace list plus execution_outcomes.
So the hourly sweep deserialises the entire history of its own approvals into ORM objects, every hour, to find the few unexecuted ones. At a pessimistic 8,760 rows a year at a few KB each that is tens of MB per tick, growing monotonically. It will not fail, and it is the same shape as the defect being fixed — a scheduled job whose per-tick work grows with history — on a much gentler slope.
The fix is a bound on time, not on count, and it is not the mistake that was just removed. The LIMIT bug was a bound on which rows the filter could see, applied before the filter, so the filter's target could be pushed out by rows it would have rejected. A window is a bound on which rows are eligible at all:
.where(Approval.decided_at >= _now() - timedelta(days=EXECUTION_WINDOW_DAYS))An approval outside it is not hidden from the filter; it is genuinely not executable. Indexable, cheap, and nothing can be pushed out by rows that were never candidates.
And it makes a claim this PR already relies on literal. The re-proof exists because "an approval authorises the disposal, it does not vouch for hours-old evidence." That protects against the evidence going stale. It does nothing about the decision going stale: a human who approved the removal of a workspace six weeks ago, on the state they were shown then, is not authorising a deletion today. A window says so in code, and ApprovalStatus.EXPIRED already exists if anyone later wants to mark them rather than merely skip them.
The trade to state honestly: a window can miss an approval granted just before its edge if the sweep is down, so it must be days rather than hours, and the count bound stays as the git-operation cap. Follow-up, not a blocker — the current code is correct and this PR is carrying enough.
The instrument was the finding, and fixing it is the better outcome
That the LIMIT test could not fail — because the fake ignored the statement, so a restored .limit() changed nothing — is worth more than the bug it missed. The fake now reads the limit two ways and applies it:
limit = getattr(statement, "_limit", None)
if limit is None:
limit = getattr(getattr(statement, "_limit_clause", None), "value", None)
if limit is not None:
rows = rows[:limit]So a restored .limit() truncates the fake's rows and test_a_backlog_of_executed_proposals_does_not_hide_the_unexecuted_one fails, which is the regression test I asked for, built exactly as asked — MAX_EXECUTIONS_PER_SWEEP executed approvals plus the one that must still be seen.
One level further down, and this is the only thing I would add to the tests. Those two attribute names are SQLAlchemy internals. If both ever move, the fake silently stops honouring limits and this test quietly returns to being unfailable — the exact state it was just rescued from, arriving through a dependency bump rather than a code change. A two-line test of the fake — hand it a select(...).limit(1) and assert it returns one row — keeps the instrument honest, and it is the same lesson one layer down: a test that cannot fail is indistinguishable from a test that passes.
The schema guard is genuinely two-sided now
repo_tests/workspace_lease_index_matches_migration_16818_test.py reads both files by path, fails loudly if either has moved (assert path.is_file()), asserts its regexes matched rather than letting a shape change yield None, and compares name and predicate for equality rather than containment. Renaming 097's index fails it. That is the claim its title makes.
The now assertion is the part I would not have thought of. Postgres rejects a volatile function in a partial index, so released_at IS NULL AND expires_at > now() would fail at migration time on a real database — and a pure string comparison between model and migration would happily pass two identically-wrong predicates. Asserting the predicate never mentions now catches the one divergence that agreeing copies cannot.
And both files it reads are .py, so **/*.py covers them in .github/filters/python-paths.yml for free. No filter entry needed — worth stating because it is the check that half this milestone exists for, and here it passes without anyone having to do anything.
The rest
- The tautology is labelled as one and its docstring names
test_acquire_reclaims_expired_leases_before_counting_capacityas the real pin. Kept as documentation, no longer claiming to be protection. - My never-re-proposed gap is its own Known gap — stated, not designed around section, called incomplete rather than finished, with the direction a later change should take. That is the correct treatment: the safe side, admitted.
- Rebased rather than force-pushed.
Verdict: land it. The time-window bound and the test-of-the-fake are follow-ups, and the second one is small enough that whoever picks up the first should take both.
|
Caretaker hold — do not merge this PR before #17738 and #17739 are folded in, even if it is green. Both issues review this PR's own content, which is not on main yet. So they are not defects in main that this PR happens to be near; they are defects that become live the moment this merges, already filed, already described, with a named fix each. Merging green would mean landing two known defects and then reopening them from the other side — and the merge would make them look like new findings against base rather than things we knew. That is worth stating on the PR rather than only in a queue, because green is the usual permission to merge and here it is not.
#17739 is the more interesting of the two as a class: a test double that reaches into a library's private structure does not fail when the library changes, it starts agreeing with everything. The I am caretaker for this PR (claimed in the ledger; the authoring session handed over). Both fixes go in one push on this branch when it comes up in the queue — it is 7th, behind #15907 and three other caretaker fixes, so there is time and there is no reason to spend a separate CI run on each. No commits from me on this branch until then. |
…hout its holder (#16818)
…hen the run ends (#16818)
… is weaker than is_live (#16818)
…removed unattended (#16818)
…about LIMIT (#16818) Closes #17738 Closes #17739 Two issues that review this PR's own content, folded in rather than left to go live on merge. #17738. The no-LIMIT design above `_approved_proposals` is right and its docstring does not draw its own consequence: an executed proposal keeps `status = APPROVED` -- execution is recorded in `context["executed_at"]` -- so it matches this query for ever, and the hourly sweep re-read its entire approval history, growing without bound. The fix is a time predicate, not a row cap. `Approval.decided_at >= _window_start()` bounds the read WITHOUT reintroducing the trap the docstring warns about, because a window does not fill up with executed rows; they age out of it. It also fixes a correctness problem the growth was hiding. A disposal approved months ago and never executed would still be executed today, against a workspace whose branch and landedness have moved on since a human looked. An approval is a decision about a state of the world, and `AUTOBOT_LLC_DISPOSAL_EXECUTION_WINDOW_DAYS` is how long that decision is assumed to still describe it -- floored at 1 day, because 0 would place every decision outside the window and disable the sweep silently. The decision #17738 asked for: aged-out approvals are **skipped, not marked**. `ApprovalStatus.EXPIRED` exists in the vocabulary and is written by nothing in this repository, and an unattended sweep transitioning a human's decision record is exactly the unattended state change this PR exists to remove -- "the sweep proposes, a human approves, nothing is removed unattended" applies to the approval record as much as to the workspace. The record is left re-approvable. #17739. The fake session read `statement._limit` and `statement._limit_clause.value` -- two SQLAlchemy internals. When either moved it would have found neither, left `limit = None`, and silently stopped applying limits, taking with it the regression test asserting a backlog cannot hide an unexecuted proposal. It reads the COMPILED SQL now, which is public and says LIMIT in every version that emits one. Three tests, and two of them exist because my first attempts could not fail: - The window test asserted only the boundary arithmetic, so removing the WHERE clause left all 16 tests green -- the fake does not filter, so a missing predicate is invisible in the returned rows. `_Session` records the statements it is given now, and the test asserts the compiled SQL carries `decided_at >=`. - The skip-not-mark test asserted `"EXPIRED" not in source` and failed on its own docstring explaining why the sweep does not write it. It parses the AST now and looks for the attribute in code, which is the same distinction between prose and code that this repo's comment guards draw. Mutation-checked, each killing exactly one test: removing the window predicate, and making the fake's limit unreadable. Rebase notes, since this branch and #17726 extracted overlapping code from `heartbeat_scheduler.py`: - Both moved `_read_file_text` and `_resolve_adapter_output_file` out, to different modules. #17726 landed first, so `replay_recording.py` on main owns them along with `record_run_for_replay`, which used them. This branch's `run_output.py` was a strict subset with one importer -- the conflicted line itself -- so it is removed. Proved before removing rather than assumed: both functions are AST-identical to main's copies. - The commit pointing the replay test at `run_output` is obsolete and was skipped: main already points that import at `replay_recording`, so its intent is satisfied by a different home. - The ceiling is 1050, measured from the resolved file, not either side's number (base said 1053, this branch 1043). Verified: 50 tests across the sweep, the stalled-run sweep and the replay resolution; 31 ratchet tests; env registry and generated docs clean.
f0f1a0d to
f105d4f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: b6a90184-5742-41a3-961e-2a6e55737986
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (19)
autobot-backend/celery_app.pyautobot-backend/llc/models/workspace_lease.pyautobot-backend/llc/scheduler/__init__.pyautobot-backend/llc/scheduler/heartbeat_scheduler.pyautobot-backend/llc/scheduler/lazy_import_test.pyautobot-backend/llc/scheduler/run_context.pyautobot-backend/llc/scheduler/run_workspace.pyautobot-backend/llc/scheduler/stalled_run_sweep.pyautobot-backend/llc/scheduler/stalled_run_sweep_test.pyautobot-backend/llc/scheduler/workspace_lease_sweep.pyautobot-backend/llc/scheduler/workspace_lease_sweep_test.pyautobot-backend/llc/services/approval.pyautobot-backend/llc/services/workspace_disposal.pyautobot-backend/llc/services/workspace_lease.pyautobot-backend/llc/services/workspace_lease_16818_test.pyautobot-backend/migrations/versions/20260928_097_workspace_lease_live_path_unique.pyautobot_shared/env_registry_llc.pydocs/developer/ENV_VARS.mdrepo_tests/workspace_lease_index_matches_migration_16818_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…E variable (#16818) `test_observability_coverage` failed on `20260928_097`, reddening four checks from one assertion: `migration-tests`, `migration-matrix`, `python-suite shard 9/12` and the `python-suite` aggregator. The assertion's hint points at `_TABLE`-variable `add_column` calls (#17125). That is not the cause here and inlining the literal would have changed nothing: 097 has no `add_column` calls at all. It drops a table-wide UNIQUE constraint and creates a partial unique index, so it creates no table and no column, and `extract_artifacts` observes revisions only through `create_table`/`add_column`. It is unobservable by construction. Extending the allowlist is the documented route for exactly this, and 097 joins an existing category rather than opening one: 20260821_081 is index-only and 20260906_089 is a constraint swap, both allowed for this same structural reason. Idempotence, which is what the allowlist actually buys with the exemption: `upgrade()` reads `get_unique_constraints` before dropping and `get_indexes` before creating, acting only where the prior state is still present, and returns early on a non-postgresql dialect or a missing table. The adoption re-run this permits is a genuine no-op, not a "constraint does not exist" or "relation already exists" failure.
The current 12 reds are #17837 (base), not this PR — and not the observability defect eitherRead-only diagnosis, posted so this is not re-derived a third time. The reds have changed cause since the last diagnosis, which is why the earlier reading no longer matches. These checks are current, not stale. Head is The failure is a dependency surface change, in base: 17 errors, all the same missing attribute, 3727 tests passing around them. That is Evidence it is base rather than this branch: Correction to the earlier diagnosis on this PR. The 12 failures were previously attributed to one defect — One instrument note worth keeping, because it is the same shape as #17839's class. These tests already defend against fakeredis being missing: try:
import fakeredis.aioredis as fakeredis_async
except ImportError:
fakeredis_async = None
...
pytest.skip("fakeredis not installed")That guard catches absence and cannot catch a surface change — an Nothing here is this PR's to fix. It clears when #17837 lands; re-run after that rather than chasing it on this branch. I am read-only and have not touched the branch, the worktree, or the claim. |
…pt (#17837) Base is red on every PR. `fakeredis` 2.39.0 reached PyPI 2026-10-01 and `requirements-ci-test.txt:21` declares `fakeredis[lua]` unpinned, so CI took it. It removed `FakeServer` from `fakeredis.aioredis`: AttributeError: module 'fakeredis.aioredis' has no attribute 'FakeServer' 215 errors across 10+ python-suite shards on #17708; 17 more on #17725. main's last green was 2b88c1c on 09-30, before the release. **A code fix rather than a pin, and the wheel says why.** Read out of the 2.39.0 wheel rather than inferred: fakeredis/__init__.py:11 from ._core import FakeServer fakeredis/__init__.py:29 "FakeServer", <- in __all__ fakeredis/aioredis.py imports only ._clients._async and ._socket The top-level export is intact and public. On 2.37.0 `FakeServer` is reachable through `aioredis` but is **not in that module's `__all__`** -- we were using an incidental re-export that was never public API. Pinning would freeze a test dependency to keep an attribute we were never entitled to. 23 files move to `fakeredis.FakeServer`, which resolves on 2.37 and 2.39 alike. **Three binding styles, two of which bit.** `import fakeredis.aioredis as X` does not bind `fakeredis`, so the import has to be added in the SAME scope: module level, inside the fixture for those that import there, and a sibling `pytest.importorskip("fakeredis")` for the four that bind dynamically, which no import-statement regex can see. My first two passes missed the latter two shapes and produced nine `NameError`s main does not have. The guarded `try/except ImportError` blocks also set `fakeredis = None`, so an absent package still skips. **One pre-existing defect surfaced and fixed, because it blocked the commit.** `workflow_redis_backfill_test.py` carries two `# canonical: ignore py-adhoc-db-engine` waivers that never applied: the rule honours a waiver only on the call's OWN line (`py_adhoc_db_engine.py:64`, `lines[node.lineno - 1]`), and both sat elsewhere. Verified against `origin/main`'s unmodified file at the same path, which reports the same 2 violations -- latent only because the hook runs on changed files and nobody had touched it. Waivers moved onto the call lines. Verified: 411 passed across a2a and llm_shared (the erroring suites), 321 across all 23 changed files, and canonical-check clean. Not fixed here: `fakeredis[lua]` is unpinned in the CI plane, the same defect that let torch 2.14.1 break every shard the day before. #17839 owns that, including the cache angle -- an unpinned line never changes the requirements hash, so a cache hit keeps the old version while a miss pulls the new one. Closes #17837 Refs #17839
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 66c0c921-337a-4eee-b0a0-0ca896752ff1
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (13)
autobot-backend/celery_app.pyautobot-backend/llc/scheduler/__init__.pyautobot-backend/llc/scheduler/heartbeat_scheduler.pyautobot-backend/llc/scheduler/lazy_import_test.pyautobot-backend/llc/scheduler/stalled_run_sweep.pyautobot-backend/llc/scheduler/stalled_run_sweep_test.pyautobot-backend/llc/scheduler/workspace_lease_sweep.pyautobot-backend/llc/scheduler/workspace_lease_sweep_test.pyautobot-backend/llc/services/approval.pyautobot-backend/llc/services/workspace_disposal.pyautobot-backend/llc/tests/test_heartbeat_scheduler.pyautobot-backend/migrations/versions/20260928_097_workspace_lease_live_path_unique.pyautobot_shared/env_registry_llc.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…he owning repo (#16818) Four CodeRabbit findings, each confirmed against the code before acting, plus the two blockers that surfaced while fixing them. 1. The proposal was never announced (workspace_lease_sweep.py) ------------------------------------------------------------- `request_approval` only adds and flushes; `publish_requested` is a separate call whose own docstring says to make it AFTER the transaction commits. `_propose` discarded the returned `Approval` and nothing in the sweep published, so `llc:approval_requested` never fired for a disposal proposal. A human approval is the only path to disposal (#17038), so the sweep proposed into silence and reported success. `_propose` now returns `(count, approval)`; `_async_sweep` publishes after `session.commit()`. 2. The promised log did not exist (workspace_lease_sweep.py) ----------------------------------------------------------- `_window_start`'s docstring states aged-out approvals are "skipped, not marked" and that "the count of skipped ones is logged rather than silently dropped". The window predicate cannot honour that: it filters in SQL, so those rows never reach the sweep and there is nothing left to count. `_log_aged_out` reads the other side of the window and warns with the ids. A SQL COUNT cannot stand in, because sweep proposals are identified by `_is_sweep_proposal`, a Python predicate. 3. `worktree remove` ran wherever the worker lived (workspace_disposal.py) ------------------------------------------------------------------------- Every other `_git` call passes `cwd=`; the removal did not, so the outcome depended on the worker's directory and git could refuse, returning UNKNOWN and leaving an approved workspace in place. The review was right about the defect, and the obvious fix fails differently: `cwd=path` is refused outright, because git will not remove the worktree it is standing in. Resolves the owner with `rev-parse --git-common-dir` and runs there. 4. A dead assertion message (the moved test) ------------------------------------------- `assert_awaited_once(), "..."` builds a tuple and discards the string. Fixed as `assert session.commit.await_count == 1, "<message>"` rather than the suggested comment-plus-call: one line instead of two, and the intent becomes a LIVE assertion message rather than a comment. Blocker A: the branch had pushed a grandfathered file past its ceiling --------------------------------------------------------------------- `test_heartbeat_scheduler.py` stood at 1144 lines against a recorded ceiling of 1075 (#14236, #5060). `origin/main` has it at exactly 1075, so the 69 lines are THIS branch's, and the ratchet refused every commit on the branch -- it checks the tree, not the changeset, so the whole PR was unable to accept work until this was resolved. Split rather than re-pinned, which is what the rule requires: the class this branch added, `TestEarlyExitsReleaseTheWorkspace`, moves to `test_heartbeat_workspace_release.py` (83 lines). The original returns to exactly 1075. No test is lost and no ceiling is raised. Blocker B: the `code-quality` required context was red on formatting ------------------------------------------------------------------- `workspace_disposal.py` was Black-dirty at the previous head -- CI said *"1 file would be reformatted"* -- which is what failed `code-quality`. Now clean, with a pre-existing 122-char line in the same file split as well, since it only becomes a lint failure once the file is in the changeset. Tests, because a fix with no test is decoration ---------------------------------------------- The fake `_Approvals` had no `publish_requested` at all, so fix 1 would have broken every proposing test. Extended it to record the publish WITH the commit state, plus three tests: the proposal publishes and does so after the commit, an empty sweep publishes nothing, and the aged-out query bounds `decided_at` from above. Ordering is asserted rather than occurrence -- publishing inside the transaction would announce a proposal a rollback could still erase. Refs #16818, #17038
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Serialise lease acquisition with approved disposal. · workspace_lease_sweep.py:205-226
autobot-backend/llc/scheduler/workspace_lease_sweep.py:205-226
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialise lease acquisition with approved disposal.
An approved proposal can refer to a path released by an earlier sweep.
_execute_approvedchecks the path and then callsdispose_workspace.acquire_leasecan commit a new lease between these operations. The partial unique index does not prevent this because it excludes the released row.Use the same transaction-scoped, path-specific PostgreSQL advisory lock in
acquire_leaseand_execute_approved. Hold the lock until disposal and the sweep transaction commit. This is separate from the existing live-lease check.Suggested fix
-from llc.services.workspace_lease import lease_for_path, reclaim_expired +from llc.services.workspace_lease import lease_for_path, lock_workspace_path, reclaim_expireddef _now(now: Optional[datetime] = None) -> datetime: return now or datetime.now(timezone.utc) +async def lock_workspace_path(session: AsyncSession, path: str) -> None: + await session.execute( + select(func.pg_advisory_xact_lock(func.hashtextextended(path, 0))) + ) + + async def acquire_lease( @@ """ moment = _now(now) + await lock_workspace_path(session, path) await reclaim_expired(session, now=moment)for entry in (approval.context or {}).get("workspaces", []): path, branch = entry.get("path"), entry.get("branch") + await lock_workspace_path(session, path) held = await lease_for_path(session, path)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c9847790-4732-4cde-becc-415cdb5bd1a3
📒 Files selected for processing (4)
autobot-backend/llc/scheduler/workspace_lease_sweep.pyautobot-backend/llc/scheduler/workspace_lease_sweep_test.pyautobot-backend/llc/services/workspace_disposal.pyautobot-backend/llc/tests/test_heartbeat_workspace_release.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
… fail (#16818) Two more CodeRabbit findings, both correct, both about code from the previous commit rather than pre-existing. Bound the aged-out approval read (workspace_lease_sweep.py) ---------------------------------------------------------- `_log_aged_out` was bounded only from above, so it read every historical approved sweep approval on every sweep. An executed proposal keeps `status == APPROVED`, so "older than the cutoff" is the whole approval history -- Python discarded the executed ones only after the database had returned them, and the hourly sweep's read grew without limit to report nothing. Bounded from below as well, one window of lookback. The cost was the finding, but the lower bound fixes a second defect it did not name: without it, a proposal that aged out in March is re-reported in every sweep thereafter, forever. That is a warning nobody can act on and everybody learns to skip, which is worse than no warning. One window reports each aged-out proposal while the fact is still news. Ids in the warning are capped at `MAX_AGED_OUT_IDS_LOGGED` with an explicit "+N more"; the COUNT stays exact, because it is the actionable part. Make the early-return guard able to fail (test_heartbeat_workspace_release.py) ----------------------------------------------------------------------------- `body.count("_release_workspace") == 3` asserts that three release calls exist, not that every exit has one. Add a fourth early `return` with no release and the count is still three: the assertion passes and the regression it exists to catch goes straight through. A guard that cannot fail, in the file that exists to stop leaked leases. Replaced with two tests that can: * `test_the_failed_early_exit_actually_releases` DRIVES the exit -- the RUNNING update raises, and the release is asserted to receive the run id and `failed`. * `test_every_return_in_run_adapter_is_preceded_by_a_release` walks each statement list and requires a release before the return in that same block, so a new unreleased exit fails by construction rather than by someone remembering to update a number. Measured shape it encodes: 3 returns at 584/616/621, each immediately preceded by a `_release_workspace` call at 583/615/620, with `_finish_run` at 623 on the normal path. Also tests the new lower bound, so the fix above is not decoration. Refs #16818, #17738
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 3972ce70-8d8f-45c6-9f53-ccc17bfd677d
📒 Files selected for processing (3)
autobot-backend/llc/scheduler/workspace_lease_sweep.pyautobot-backend/llc/scheduler/workspace_lease_sweep_test.pyautobot-backend/llc/tests/test_heartbeat_workspace_release.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
… readable (#16818) Three CodeRabbit findings, all on code from my previous two commits. The assertion message printed its own placeholder ------------------------------------------------- `"...aged out arbitrarily long ago:\n{sql}"` had no `f` prefix, so a failure would have printed the literal `{sql}` instead of the query. Worse than flagged: the `\n` was escaped too, so the message carried a literal backslash-n as well. Both fixed. This is the aged-out-bound test from the previous commit -- a guard whose failure message cannot be read is most of the way back to a guard that cannot fail, since the diagnosis it exists to hand over is the part that goes missing. Nested `with` (SIM117) --------------------- Two patches, one `with` holding both contexts. `_is_release` read its node before deciding what the node was ------------------------------------------------------------- It computed `node.value.value` first and checked `isinstance(node, ast.Expr)` last. The review is right that the RESULT was correct -- the trailing check rejected anything wrong, so an `ast.Assign` whose value is an `Await` could not slip through -- but the order made the reader verify that by hand. Now it returns early unless the node is `Expr(Await(...))`, then reads the call, which is the order the question is actually asked in. Return annotation added (ANN202) and a one-line docstring, so the helper states its own subject. Refs #16818
Note on state: the three resolved threads are fixed in a commit that is not pushed yetFlagging this rather than leaving it implicit, because the PR currently reads more ready than it is. The three threads resolved just now each cite That is deliberate and it is the queue discipline working as intended: a push is a CI relaunch, the gate has this PR at But the combination is a shape worth naming, because it is the same one that produced a true-green-on-defective-content earlier in this queue: a fix held back plus state that advances around it equals a PR that looks readier than it is. There the branch advanced while the fix waited; here the thread state advanced while the fix waited. Resolved threads are read as "addressed", and for the moment they mean "answered, with the fix committed elsewhere". Concretely, until the next push appears on this branch:
None is a correctness defect in shipped behaviour — one is a failure message, two are readability — which is why holding them is safe. A correctness fix would not have waited; the queue rules exempt those from any hold, and that exemption exists for exactly this reason. |
The two reds are one defect: a grandfathered file grown past a ceiling with zero headroomRead-only diagnosis at head Two things stack, and only one of them is this branch's doing
So the branch is behind main on a hub file. But merging main does not clear this. The ceiling becomes 1053 and the file is 1076. The post-merge size is not something I can state exactly — if #17726's extraction removed lines this branch also carries, the merge will reduce it — so it needs re-measuring after the merge rather than predicting. What is certain is that 1053 is the ceiling it must come in under, not 1050. The ceiling has no headroom by constructionMain's file is 1053 and its recorded ceiling is 1053. Exactly equal. #17726 reduced the file and re-pinned to the new exact size, so any addition to this file fails the ratchet — this branch's 68 added lines are not unusually large, they are the first change to arrive after the pin was tightened to zero margin. That is the same shape as the saturated-floor class #17818 owns and as #17870: a pin set to the current measured value leaves no room, and the next legitimate change pays for it. Worth knowing so this does not read as carelessness in the branch. The remedy is extraction, not a higher ceiling
The added content suggests its own seam. The Noting for whoever does it: the branch already absorbed a review finding in this file — the Not baseTo be explicit about the direction, since I checked it: Read-only — I have not touched the branch, the worktree or the claim. |
…e ratchet pins (#16818) Clears both reds on #17725. They had opposite remedies, so both were needed. code-quality: stale base, not this branch ----------------------------------------- mypy errors in `context_aware_decision/collectors/main.py` and `api/code_sync.py`. Neither is among this branch's changed files and main is green on both, so merging main is the entire fix. python-suite: the ratchet, and the pin had to be reconciled first ----------------------------------------------------------------- `heartbeat_scheduler.py` was 1076 against a ceiling of 1050, and 1050 was UNREACHABLE: main's own copy of the file is 1053 lines, so satisfying it would have meant deleting three lines of base's unrelated code. Neither pin was wrong when written. At `2f6369e006` this branch had the file at exactly 1050 and pinned 1050; main independently reached 1053 and pinned 1053. Two true records of two diverged trees. The merge has to pick one, and base's is the authoritative one for the merged tree. There are TWO records of this ceiling and only one is read by the guard: `scripts/python_file_size_known_large.py` (the hook's source) and `repo_tests/python_file_size_ratchet_baseline.py`. Editing the second alone looked like a fix and changed nothing -- the guard kept reporting 1050. Both are now set to the same value; the duplication itself is worth a separate issue. Extraction, which is what the rule asks for instead of a higher ceiling ----------------------------------------------------------------------- Two seams, both pulled out rather than re-pinned: * `_release_workspace` -> `run_workspace.release_workspace_best_effort`, consolidating into the module that already owns `acquire_run_workspace` and `release_run_workspace`. Its reasoning moves with it. * the mark-RUNNING prologue -> `run_status.mark_running`, a new module beside `run_context` and `run_workspace`, which hold the other per-run concerns. It needs no `self`, so it was never a method in substance. That also fixes a function-length red the move surfaced: `_run_adapter` was 67 body lines against a 65 limit (#620). It had been over for a while and the guard skips long functions a change does not touch, so touching it is what revealed it. Ceilings lowered to the achieved 1028, in both records. The ratchet turns down. The release stays with its caller, deliberately ----------------------------------------------- `mark_running` writes the FAILED status on failure -- that belongs with the attempt that failed -- but does NOT release the workspace. The caller does, so that every exit from `_run_adapter` has its release visible to a reader of `_run_adapter`. Moving it inside would have satisfied the line count and hidden the thing #17725's review was about. Tests retargeted, and the structural guard re-proved ---------------------------------------------------- The guard matched `call.func.attr == "_release_workspace"`, an ast.Attribute. The calls are now a module-level ast.Name, so it would have found zero release calls and flagged every return -- failing loudly, for the wrong reason. Re-verified by mutation rather than by assumption: real code -> no returns without a release bare `return` injected at :620 -> detected Refs #16818, #17725, #620
…ssue-16818-lease-wiring
…counted two (#16818) `python-suite shard 5/12` failed on `test_run_adapter_applies_backoff_for_registry_rate_limit` with `AttributeError: 'coroutine' object has no attribute 'all'`. The production path is correct; the test's session arithmetic was not --------------------------------------------------------------------- Its factory returns the first context manager on call 1 and the second on EVERY later call. `_run_adapter` now opens three sessions on the rate-limited path: 1 mark_running SELECT retry_count + UPDATE RUNNING 2 _handle_rate_limited UPDATE ... status = rate_limited 3 release_workspace_best_effort hands the workspace back (#17725 review) so the release reused session 2 and two things broke. `execute.call_args` returns the LAST call, so the lease query became the subject of the RATE_LIMITED assertion -- which is why the failure message compared `'rate_limited'` against a `SELECT llc_workspace_leases...`. And session 2's result is a bare `AsyncMock`, whose auto-created `.scalars` is itself async, so `result.scalars()` returned a coroutine and `.all()` raised. Session 1 already used a `MagicMock` result for exactly that reason. NOT the extraction. `mark_running` replaced an inline `async with factory()` with a function that opens one session -- same count, relocated. The third session is the LEASE RELEASE this PR adds on purpose: a rate-limited retry is re-dispatched as a new run that acquires its own lease, so holding this one keeps the directory unusable until its deadline. The test predates that behaviour. The fix also covers the behaviour rather than just restoring green ----------------------------------------------------------------- A third mock with a correctly shaped result, and an assertion that the exit actually touched `llc_workspace_leases`. Asserting only the status write would have gone green while saying nothing about the release -- the thing the review asked for. Mutation-proved rather than assumed: release call present -> 21 passed release call removed -> 1 failed (this test) Refs #16818, #17725
…17872) Finding 1 of the issue, which the first two commits left standing: a lowered baseline failed a test called `test_no_ceiling_may_be_raised`, and nothing had been raised. The reader is sent after a cause that did not occur, which is worse than being told only that something disagrees. `ceiling > baseline` is produced equally by a raised ceiling and a lowered baseline -- the predicate cannot distinguish them -- so neither the name nor the message claims one any more: test_no_ceiling_may_be_raised -> test_no_ceiling_may_exceed_its_baseline "ceilings raised (now, baseline)" -> "the hook's ceiling exceeds its baseline (hook, baseline) ... which record moved is not established here" The predicate itself is unchanged and still one-directional on purpose. The reverse direction is the parity test's job; writing it in both places would make two records of one check, which is the shape this issue is about. Mutation-verified in both directions, which is also what proves the new file is load-bearing rather than redundant: hook ceiling 910 -> 911 both tests fail, both reporting (911, 910) baseline 910 -> 960 the renamed test PASSES; only the parity test fails, reporting (910, 960) -- the #17725 direction both files restored, md5 matches, 37 passed One more thing the reviewer should see: `python_file_size_ratchet_test.py` was at 589 of the 600-line hard limit with no KNOWN_LARGE entry, and my first version of this change took it to 608. Adding an entry to the hook's record to accommodate that would have been raising a ceiling to fit a docstring. So the rationale moved into the parity test's docstring, where there is room, and this file keeps a one-line pointer and 5 lines of headroom.
…d stop the cross-check naming a cause it cannot establish (#17872) (#17882) * test(gates): compare each gate's two ceiling records for EQUALITY, not one direction (#17872) The duplication is NOT the defect, and #17872's original ask was wrong --------------------------------------------------------------------- I filed that issue asking for one record to be derived from the other. Reading the files says not to. Both baselines document the two copies as a mechanism: python: "neither copy is derived from the other at runtime ... a list only ever compared against itself can drift anywhere ... no ceiling can be raised by editing one file alone" shell: "Two copies is the point, not an accident ... The mirror is what makes a shrink irreversible: the gap between a lowered hook and an unlowered mirror is exactly the lines just cut" The hook's copy is anchored to real file sizes by `audit_ceilings`; the baseline is anchored by the ratchet test. Deriving one from the other collapses two independent anchors into one and destroys exactly the protection that makes a raise detectable. Derivation would have been a regression dressed as cleanup. The real defect: the comparison was asymmetric on the Python side ---------------------------------------------------------------- The mechanism only holds if the two records are compared BOTH ways. They were not. `test_no_ceiling_may_be_raised` fires on `ceiling > baseline` alone, and `test_no_entry_may_be_added` compares keys in one direction and skips `rel not in RATCHET_BASELINE` silently. There was no equality assertion at all. So raising the BASELINE alone passed every check -- the direction that actually happened, on #17725: I edited the baseline, confirmed the new number was in the file, and the hook kept enforcing the old one. An edit that looks applied and changes nothing. The shell gate has had the equality assertion all along ------------------------------------------------------- `shell_file_size_ratchet_test.py` asserts `hook.KNOWN_LARGE == RATCHET_BASELINE` outright, names both files, and asserts no direction. Fourth time in this batch that the shell copy was the correct one. Asserted for BOTH pairs rather than only the broken one: the defect is two hand-kept copies per gate with an asymmetric comparison, and fixing one gate leaves the identical arrangement one directory away with nothing pointing at it. The data-source difference -- 479 entries loaded from a module versus 10 declared inline -- is a parameter, not an obstacle: the hook is the authority either way. Demonstrated, not argued ------------------------ Raised one baseline entry alone, 713 -> 763, hook untouched: test_no_ceiling_may_be_raised PASSED <- the existing check misses it test_the_two_records_are_equal FAILED <- this one catches it Both green again on revert. The message names both values and asserts no direction, because lowering a baseline used to fail a test called "no ceiling may be raised" -- sending the reader after a raise that never happened. A wrong diagnosis costs more than a vague one. Touches no baseline file and no gate, so it does not collide with #17865. Refs #17872, #14236, #17353 * test(gates): drive the disagreement message on synthetic records (#17872) The issue's last criterion asks for a test that proves the one-sided edit fails "with the message asserted too, since the message is the whole deliverable here". The first commit did not deliver that. I had mutation-verified the one-sided edit by hand -- raise the baseline alone, watch this file go red -- but a check I ran once in a worktree is not a check the repo keeps running, and the AC asks for the latter. Ticking it on my own transcript would have been evidence of the wrong kind. The obstacle was that a message only ever produced by a real disagreement in the checked-in records cannot be asserted on without breaking the tree. So the comparison moves into `_disagreement_report(known, recorded, ...)`, which takes its two records as arguments; the real pair feeds it the checked-in records, and three new tests feed it synthetic ones: * a one-sided edit (1050 vs 1028, the exact #17725 shape) must name the file, carry BOTH values in (hook, baseline) order so the reader knows which file to edit, name both records, and headline the symmetric fact rather than a direction -- the old failure said "no ceiling may be raised" about an edit that raised nothing; * a key in one record only, BOTH directions -- the Python key check covered one and skipped the other in silence; * agreeing records produce no report at all. Without that contrast case a reporter that always complains would pass both tests above. Mutation-verified, each mutation confirmed applied by grep before trusting the result: swap (hook, baseline) order -> 1 failed (the both-values test) drop only_in_baseline -> 1 failed (the baseline-only case) reporter always returns "" -> 3 failed (both failure tests + ...) restored, md5 matches -> 6 passed * test(gates): stop the cross-check naming a cause it cannot establish (#17872) Finding 1 of the issue, which the first two commits left standing: a lowered baseline failed a test called `test_no_ceiling_may_be_raised`, and nothing had been raised. The reader is sent after a cause that did not occur, which is worse than being told only that something disagrees. `ceiling > baseline` is produced equally by a raised ceiling and a lowered baseline -- the predicate cannot distinguish them -- so neither the name nor the message claims one any more: test_no_ceiling_may_be_raised -> test_no_ceiling_may_exceed_its_baseline "ceilings raised (now, baseline)" -> "the hook's ceiling exceeds its baseline (hook, baseline) ... which record moved is not established here" The predicate itself is unchanged and still one-directional on purpose. The reverse direction is the parity test's job; writing it in both places would make two records of one check, which is the shape this issue is about. Mutation-verified in both directions, which is also what proves the new file is load-bearing rather than redundant: hook ceiling 910 -> 911 both tests fail, both reporting (911, 910) baseline 910 -> 960 the renamed test PASSES; only the parity test fails, reporting (910, 960) -- the #17725 direction both files restored, md5 matches, 37 passed One more thing the reviewer should see: `python_file_size_ratchet_test.py` was at 589 of the 600-line hard limit with no KNOWN_LARGE entry, and my first version of this change took it to 608. Adding an entry to the hook's record to accommodate that would have been raising a ceiling to fit a docstring. So the rationale moved into the parity test's docstring, where there is room, and this file keeps a one-line pointer and 5 lines of headroom. * test(gates): the parity guard had the defect it was written to catch (#17872) Review on #17882 found it, and it is the third variant of the same class tonight -- the first one inside a guard built specifically to close that class. `test_the_two_records_are_equal` passed when BOTH records were empty, because `{} == {}` is True. Ask what that PASS licensed: "the two ceiling records agree", on the strength of having read neither. Measured before fixing, emptying only each mapping so the result isolates emptiness rather than a broken module: before 3 failed, 34 passed all three in python_file_size_ratchet_test.py; this file PASSED after this file FAILS, naming both records So the suite was not blind -- test_recorded_ceilings_match_the_files_today and two others do fail on an emptied record -- but a guard has to fail for its own reason, and this one reported agreement. A non-empty assert, deliberately NOT a declared floor ----------------------------------------------------- Both records only ever shrink; the hook's header says so. A static floor near today's size fails the first time a file is legitimately split, and a floor an order of magnitude below it is the number-chosen-by-feel that `declare`'s own docstring names as that mechanism's recurring failure. A floor needing to be lowered every time the ratchet succeeds is an incentive against doing the work. Nor is partial degradation reachable: each record is a literal dict run by `exec_module`, so a broken loader raises rather than returning a subset. Empty is the only silent degradation, and empty is what this catches. The reasoning is in the code so it is not "simplified" in either direction, and it says what would reopen the floor question -- a loader that parses or filters. The tree-anchored direction stays where it already lives: every file over MAX_LINES must be recorded, owned by test_recorded_ceilings_match_the_files_today. Asserting it here too would make two records of one check. Mutation-verified both ways: both python records emptied -> 1 failed, message names both records reach check always returns "" -> 3 failed; the contrast case still passes restored -> md5 matches, 56 passed `test_populated_records_produce_no_reach_failure` is that contrast case: without it a reach check that always complains would pass the three failure tests -- the same hole one level down. * test(gates): one owner for ceiling equality, and two files per comparison (#17872) Review found this PR's own thesis turned on itself: parametrising over "shell" made a SECOND record of a check `shell_file_size_ratchet_test.py` already performed -- `test_the_two_copies_of_the_ceilings_agree` asserted `hook.KNOWN_LARGE == RATCHET_BASELINE` for that gate already. Why I missed it is worth recording: Python has NO equality test (31 `def test_` in its ratchet file, none asserting equality -- that absence is why #17872 was filed), and shell had one. Reasoning about the file I was editing was right for Python and silently wrong for shell, so parametrising generalised the gap along with the fix. I had asked exactly this question of the tree-anchored direction and declined to duplicate it; I never asked it of the shell file. The old shell test is removed and this file now owns equality for both gates. Mine is strictly stronger, and that is measured rather than asserted: shell hook ceiling 818 -> 800 (one side only) -> [shell] fails: disagreeing (hook, baseline) {'scripts/pr-preflight.sh': (800, 818)} shell hook record emptied -> [shell] fails the reach check, naming scripts/check_shell_file_size.py The deleted test said something neither of mine did -- the gap between a lowered hook and an unlowered mirror "is exactly the lines just cut, and it is spendable". That is what a disagreement MEANS, so it moved into the failure message a reader actually sees, not into a comment. Also found while in here: equality passes trivially if both halves of a pair resolve to ONE module, which is precisely "a list only ever compared against itself can drift anywhere" -- the baselines' own warning, reachable through my own fixture. Two files and two objects are now asserted before the comparison. Smaller items from the same review: * `_empty_record_failure` was 34 lines of which ~8 were body; the rationale moved to the module docstring. Longest function in the file is now 29. * the `_PAIRS` comment quoted "479 entries" and "10 inline". Both shrink by design, so both would go stale -- dropped, for the same reason a numeric floor was rejected. * `test_no_ceiling_may_exceed_its_baseline`'s message said "Lower BOTH in the same commit", which is wrong for ITS direction: when the hook exceeds the baseline, lowering both RECORDS the raise instead of undoing it. It now says lower the hook to its baseline, or find the raise that put it above. Left alone deliberately, with the reason recorded in the module docstring so it is not re-derived: both gates' exceed-direction tests are logically subsumed by this equality, but each gives the common direction a specific diagnosis, and the redundancy is pre-existing and symmetric rather than introduced here. 55 passed (56 before, minus the deleted duplicate). Both gates exit 0.
1 participant
Thinking Path
#16822 gave the workspace a model, a migration and a model-level test, and stopped there. Nothing
acquired a lease, nothing released one, nothing reclaimed one — the table would have stayed empty
for ever. This is the wiring that makes the record mean something.
Three decisions shaped it.
Freeing the slot and removing the directory are separate steps. Reclaiming a lease returns
capacity unconditionally; disposing of the directory requires evidence that the work reached the
remote. Fusing them gives only bad answers: either a workspace with unpushed work blocks the
reclaim — which is the original deadlock — or it gets removed to free the slot, which loses the
work. Split, the sweep can say "slot back, directory stays, here is why".
Disposal has three verdicts, not two.
LANDEDauthorises removal;NOT_LANDEDandUNKNOWNboth refuse, for different reasons. Collapsing
UNKNOWNintoLANDEDwould delete a workspaceevery time git happened to error — silently, permanently, and indistinguishably from correct
operation. They are kept apart so nobody later simplifies them in the wrong direction.
Removal is
git worktree removewith no--force. Git refuses a worktree with uncommittedchanges, and that refusal is information, not an obstacle. Forcing it would convert every one of
those signals into data loss.
One lease site, not four. The four subprocess adapters each resolve
workspace_dirthemselves.Leasing in each would mean four acquire sites and four chances to forget one — the shape that
produced this defect. The lease is taken once in the scheduler, where the run is created.
What Changed
llc/services/workspace_lease.pyllc/services/workspace_disposal.pygit worktree removewithout--forcellc/scheduler/workspace_lease_sweep.pyllc/scheduler/run_workspace.pyllc/scheduler/stalled_run_sweep.pyllc/scheduler/heartbeat_scheduler.pymigrations/.../20260928_097_*.pyrun_context.py,run_output.pyheartbeat_scheduler.pyto stay under its ceilingA defect in the merged model, fixed here
093 created
uq_llc_workspace_leases_pathas a table-wide UNIQUE. The model's own comment besidethat column says what was meant: "two live leases on one directory is the collision the ledger
exists to prevent". The comment says live; the constraint said ever. A path could be leased
exactly once in the lifetime of an installation — the first release permanently burned that
directory, and since workspace paths are derived from issue numbers and reused constantly, that is
every path on its second use. 097 replaces it with a partial unique index carrying the intent the
comment always stated. Nothing depended on the stricter reading, because at 093 nothing acquired a
lease at all.
Reviewing 097: read the comment beside the column, not just the migration. The migration on its
own looks like it is loosening a constraint, which is the kind of change a reviewer is right to be
suspicious of. What makes it correct is that
workspace_lease.py's comment onpathalready statesthe intended rule — live, not ever — so this brings the schema to the documented intent rather
than relaxing it to suit the code. The disagreement was between the comment and the constraint; the
code is what exposed it, by being the first thing to ever take a lease.
A behavioural change worth naming
A run whose workspace is already leased does not dispatch — it re-queues. Two agents in one
worktree corrupt each other's work, which is exactly what the lease exists to prevent; dispatching
anyway would make the lease a record of the collision rather than a guard against it. The lease's
own expiry is what guarantees this cannot wedge an agent permanently, and
acquirereclaims expiredleases before counting capacity, so the ceiling clears itself.
What this does not cover
Session-owned worktrees are still outside the domain. Nothing in
llc/creates a worktree —adapters are handed a
workspace_dir. So this leases the workspaces agent runs use, and theworktrees that sessions create for themselves are untouched by it. The 2026-09-16 deadlock was
three merged worktrees held by a dead session, and AC4 points at that case explicitly, so I am
not claiming it closed:
worktree-reap.shstill owns that population, and bringing it into thedomain needs a decision about whether session workspaces are Company OS's to model at all.
Everything else in the AC list is delivered for agent workspaces. Refs rather than Closes for that
one reason.
Verification
Owner ruling: the sweep proposes, a human disposes (#17038)
The first version of this sweep removed directories itself once it had proved the work was on a
remote. The proof was sound; the automation was not. An agent removing data on its own leaves a
logger.infoin a worker as its only record, and that is not a paper trail. Per the ruling, thesweep now writes a disposal proposal to the approval queue with the landedness evidence attached,
and stops.
What that changes:
on the record they are approving, not in a log they cannot reach.
hours ago describes the directory as it was hours ago; someone may have worked in it since. The
approval authorises the removal — it does not vouch for the evidence — so the check reruns and can
still refuse, and that refusal is written back onto the approval.
company_idisnullable for exactly this case, and guessing would put one company's directories in front of
another company's reviewer.
requested_byis the stringllc-workspace-lease-sweep. There is no agent here, and minting aUUID to satisfy a type hint would put a fabricated identifier in an audit record. The column is
String(255), documented for "a plain string for a platform-general requester" — the service's hintwas narrower than its own column, and is widened to match.
Reclaiming is unchanged and still needs no approval: it is a database write about a lease, touches no
directory, and is what frees capacity. The slot comes back on the sweep's authority; the directory
waits for a person.
Review round 2 — a silent stop, and two tests that could not fail
The blocker: a SQL
LIMITahead of a Python filter. An executed proposal keepsstatus = APPROVED— execution is recorded incontext["executed_at"], because the approvalvocabulary is six values shared by every gate and inventing a seventh to fix a query would change
all of them. So executed rows match the query for ever. With
.limit(20)in SQL, once twentyexecuted proposals existed the limit would fill entirely with them, the filter would drop all
twenty, and no approved disposal would ever execute again — reporting
disposed 0, refused 0,which is exactly what a sweep with nothing to do reports. Cannot see the work and there is no
work, rendered identically, in the sweep written to stop that.
Fixed by removing the SQL
LIMITentirely, ordering bydecided_at, and applyingMAX_EXECUTIONS_PER_SWEEPafter the filter — where the constant's own comment always said itbelonged: a cap on git operations per tick, not rows per query. No limit is pushed into SQL because
any bound applied before the filter reintroduces the same class.
My test for it could not fail, and I caught that by mutation-checking it. The first version
passed with the bug restored, because the fake session ignored the statement — so a SQL
LIMIThadno effect in the test either way. The fake now honours
LIMIT, and the test fails with the defectand passes without it. That is the same finding 87 raised about the schema test, in my own new test,
one commit later.
The model/migration comparison moved to a real guard. It compared the model to literals written
in the test, so editing 097 left it green. It is now
repo_tests/workspace_lease_index_matches_migration_16818_test.py, reading both sides fromtheir own files, comparing the predicate exactly rather than by substring —
released_at IS NULLisa substring of
released_at IS NULL AND expires_at > now(), which is both a divergence andimpossible as a partial index — and asserting the predicate can never reference
now.Mutation-checked: renaming the migration's index fails it.
The tautology is labelled as one.
test_the_partial_predicate_is_weaker_than_is_live...assertstwo things true by construction, with no index and no database. Its docstring now says so and names
what actually pins the invariant, rather than reading as protection it does not provide.
Known gap — stated, not designed around
candidatesholds only the leases reclaimed this run, andexecuted_atretires an approvalwhatever the outcome. So a workspace that fails
work_landedat proposal time, or refuses atexecution, is never proposed again: it keeps its directory, its lease is already released, and the
only record is a log line. It becomes exactly the debris this sweep exists to remove.
That is the safe direction — nothing is deleted — but it is incomplete, not finished. A later change
should re-examine unreleased-but-undisposed workspaces rather than only newly reclaimed ones.
Review round 1 (87's findings, both fixed in this PR)
The model was not in the diff, and would have reproduced the bug. 097 dropped
uq_llc_workspace_leases_path, butworkspace_lease.pystill declaredunique=Trueon the column— so a database built from the models rather than by alembic (a fixture's
create_all, a fresh devbox) carried exactly the constraint the migration removes. The model now declares 097's partial
index in
__table_args__, and three tests compare the two: no table-wide unique on the column, thepartial index present by name and predicate, and the gap below.
The partial predicate is weaker than
is_live(), and cannot be otherwise.is_liveisreleased_at IS NULL AND expires_at > now; a partial index cannot callnow(), so the index canonly express the first half and permits a second lease on a path whose lease has expired but not
been released. That gap is closed by convention, not schema: every writer reclaims before
inserting, which
acquire_leasedoes as its first statement. The migration docstring previouslyimplied equivalence; it now states the invariant, in the file the next person writing an insert path
will actually read.
Verification — updated
Most of this now has run locally. The 3.11 floor applies to
llc/scheduler/base.py, which thesetests do not import, so they execute on 3.10:
Eleven of those cover the proposal behaviour, including the one that would catch a regression to the
old design: a freshly reclaimed lease must reach a reviewer, not
dispose_workspace.Caveat the harness prints itself: 91 of 139 dependency floors are below declared in this
environment, so a local pass carries no general guarantee about CI. These particular tests are
SQLAlchemy metadata and pure-Python fakes, so the divergence does not reach them — but CI remains the
authority.
The original push was not verified: The pre-push hook reported it
verbatim rather than my paraphrase:
Pushed with
AUTOBOT_PREPUSH_ALLOW_UNRUNNABLE=1, which the hook documents for exactly this case andexplicitly distinguishes from
--no-verify. CI on 3.14 is the verification for every test claimbelow — I have not run them and am not asserting they pass.
All pre-commit hooks ran and pass. Two of them caught real defects in this change:
git-toplevel-env-scrubbed— my_githelper inherited the caller's environment.GIT_DIRoutranks
cwd=, so the disposal check would have answered about whatever repository theenvironment named, with a zero exit code. A
LANDEDverdict read off the wrong repositoryauthorises removing a workspace whose work was never pushed. Fixed with
env=scrubbed_git_env().destructive-migration-marker— 097 dropped a constraint without aNO DATA LOSSstatement.Tests written (25 across three files), the two AC6 names among them:
second hands
reclaim_expireda live lease directly, which is what a mistypedWHEREwoulddo, rather than letting the database do the filtering and proving nothing about the code
UNKNOWNrefuses disposal; a git failure isUNKNOWN, not a clean miss--force, and touches nothing when the check refusesheartbeat_scheduler.pywent 1078 → 1043 lines; both recorded ceilings lowered to match, since theratchet only turns down.
Model Used
Claude Opus 5 (1M context)
Refs #16818
Refs #16817
Summary by CodeRabbit
New Features
Bug Fixes