feat: time-boxed locks — lock a belief for a bounded window, then let it age out - #1325
Conversation
Nullable ISO-8601 UTC expiry for a user-lock, NULL meaning no expiry. Every existing row migrates to NULL and is behaviourally identical to today's permanent lock, so this commit is storage round-trip only — nothing reads the column yet. No CHECK on the ALTER, matching the convention the neighbouring entries document: ALTER TABLE ADD COLUMN cannot carry one reliably across SQLite versions. There is no enum to enforce here in any case.
Expiry is materialized by an idempotent open-time sweep rather than evaluated as a predicate. `lock_level` is read in roughly fifteen places, and `list_speculative_beliefs` is the *complement* of `list_locked_beliefs` — a `now`-aware predicate applied to one and not the other would drop an expired lock out of both tiers at once, making it invisible to L0 and L1 alike. After the sweep every existing `lock_level` read is correct unchanged. Only `lock_level` moves. `origin` is untouched, because the belief was user-asserted and only its injection privilege expired; rewriting it would reproduce the autolock origin-laundering defect. `lock_expires_at` and `locked_at` are retained as the trace of why the belief is now unlocked, so a non-NULL expiry does not imply "still locked". One `lock:expire` row per flip lands in the table `aelf unlock` writes to, so `aelf feed` shows the transition. Measured before choosing the shape, per the issue: the sweep predicate is a full `SCAN beliefs` at 2.18 ms median on a 44,594-belief store, and this runs on every open of a latency-gated path the hook opens several times a turn. A partial index on the same `IS NOT NULL` term the query states verbatim takes it to 0.001 ms, so the index is not optional. With nothing due the sweep is one index probe and takes no write lock. Runs under `_run_guarded_migration`: a failing sweep leaves a lock injected one session too long, which beats a store that will not open. The issue names the complement `list_unlocked_beliefs`; no such method exists. The argument is sound against `list_speculative_beliefs`, which is what actually selects `lock_level = 'none'`.
`--for <N>[d|w|mo|y]` (plus `forever`) and `--until <date|timestamp>`, mutually exclusive, both resolved to an absolute UTC instant at write time. A relative expression is a way of *saying* an instant, not a thing to store: written as-is it would silently re-anchor to whenever it was next read. Parsing happens before the store is opened, so a malformed window fails without having first written a permanent lock the user has to notice and undo. A `--until` in the past is rejected rather than accepted and swept on the next open, and a zero-length `--for` is rejected as a scripting mistake rather than read as "expire immediately". Calendar units use calendar arithmetic: 1mo from January 31 is February 28, and 1y from February 29 is February 28. Only d and w are fixed length. There is deliberately no bare `m` — it reads as either minutes or months. Re-locking with `--for` refreshes the window. Re-locking *without* one clears the expiry and says so; that is the single ambiguous case in the surface and the issue decides it, so this does not have to guess. `aelf unlock` clears the expiry outright — the user revoked the lock, so the window governing it is moot, which is why this differs from the sweep, where the window is the explanation for the unlock. `aelf locked` gains an always-present window column so the values line up down the listing. `expired` is not a value it can render: the open-time sweep has already flipped anything due.
The failure mode of a time-boxed lock is it vanishing silently at the moment it mattered — the user asked for seven days, got seven days, and has no idea day eight arrived. `aelf doctor` now reports both halves, and neither substitutes for the other: the forward warning names locks closing within seven days, which is the chance to extend before they go; the backward line reports the last sweep that actually fired, which is the only place an expiry that already happened is visible at all. Both informational — a lock reaching the end of a window the user chose is the mechanism working, not a fault. `aelf_lock` gains `expires_in` taking the same grammar as `--for`, resolved through the same parser so chat and shell cannot drift on what "for the next week" means. Absence of the argument on a re-lock clears an existing window, matching the CLI. The locked listing returns `lock_expires_at`, null for a permanent lock.
Every guard verified to go red under a mutation, so none of them is passing vacuously: - drop the `lock_level = 'user'` term from the sweep and the never-locked test fires — without it the sweep re-audits every unlocked row carrying a retained expiry, on every open, forever; - clear `origin` in the sweep and the preservation test fires; - approximate a month as 30 days and the calendar test fires on January 31; - accept a past `--until` and both the parser and CLI tests fire; - apply the window only on the new-lock path and all three re-lock tests fire. The both-tiers test asserts presence *and* absence deliberately. Absence from the locked list alone is equally satisfied by a deleted belief or by one that fell out of every tier — which is precisely the bug a `now`-aware predicate on one side of the complement produces, and the reason the sweep exists. Driven in-process through `main(argv, out=...)` rather than by subprocess: nothing here needs process isolation, and a subprocess would be one more blocking call for the termination policy to bound. Dotdir and DB are pinned per-test — the live store is repo-local, so an unpinned test would sweep the developer's own locks and still pass CI.
States the two things a reader would otherwise get wrong: that nothing decays (an expired lock re-enters ranked retrieval, where existing age-discounting already handles it — no new machinery), and that expiry is a sweep rather than a predicate, with the complement-of-the-lock-list reason that makes the predicate approach unsafe.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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 |
Reviewer's GuideImplements time-boxed locks by adding an expiry column and an open-time sweep, CLI/MCP surfaces for specifying windows, visibility tooling, and comprehensive tests and parsing utilities, all without changing existing lock semantics or decay machinery. Sequence diagram for open-time lock expiry sweepsequenceDiagram
participant App as Store.__init__
participant Store
participant DB
App->>Store: _run_guarded_migration(sweep_expired_locks)
Store->>Store: sweep_expired_locks()
Store->>DB: SELECT id FROM beliefs
Note right of Store: WHERE lock_expires_at IS NOT NULL
Note right of Store: AND lock_expires_at <= ts
Note right of Store: AND lock_level = LOCK_USER
Store->>DB: UPDATE beliefs SET lock_level = LOCK_NONE
Store->>DB: INSERT INTO feedback_history
Store->>DB: INSERT OR REPLACE INTO schema_meta
Store->>Store: _commit_mutation()
Store-->>App: return flipped_count
Sequence diagram for time-boxed lock creation via MCP and CLIsequenceDiagram
actor User
participant CLI as _cmd_lock
participant MCP as tool_lock
participant Expiry as lock_expiry.parse_for
participant Store
User->>CLI: aelf lock "<text>" --for <window>
CLI->>Expiry: parse_for(lock_for, now=datetime.now())
Expiry-->>CLI: expires_at (ISO) or error
CLI->>Store: _open_store()
CLI->>Store: update_belief(lock_expires_at=expires_at)
User->>MCP: aelf_lock(statement, expires_in)
MCP->>Expiry: parse_for(expires_in, now=datetime.now())
Expiry-->>MCP: expires_at (ISO) or LockExpiryError
MCP->>Store: tool_lock(..., expires_in)
Store->>Store: _apply_window()
Store->>Store: update_belief(lock_expires_at=expires_at)
Store-->>MCP: {kind, id, action, expires_at}
Flow diagram for lock expiry visibility in aelf doctorflowchart TD
A[aelf doctor] --> B[_open_store]
B --> C["list_expiring_locks(before)"]
B --> D["last_lock_sweep()"]
C --> E[_print_doctor_lock_expiry]
D --> E
E --> F[print expiring locks within LOCK_EXPIRY_WARN_DAYS]
E --> G[print last sweep timestamp and count]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Setr:2026-08-04T05:54:26Z] |
#1314) An expired time-boxed lock retains `lock_expires_at` as the audit trace of why it is unlocked, so a non-NULL expiry does not imply "still locked". Every path that re-locks an existing belief therefore has to clear it: setting lock_level='user' while a past expiry is still on the row hands the next open-time sweep a due row, and the lock is flipped straight back off. `aelf lock` and the MCP tool both clear it. `hook._autolock_candidates` and `review.apply_decisions`'s `lock` verdict construct the same field update by hand and did not, so both reported success and were silently undone at the next store open -- auto-lock printing "auto-locked <id>" on the way. That is the silent-vanishing failure mode the feature exists to prevent, reached through the two surfaces that lock without a window. Reproduced against a real store before fixing: belief locked with a past window, swept to unlocked, re-locked through each path, read back `user`, then `none` after one reopen. Both now read `user`. Neither surface accepts a window, so the lock they grant is permanent -- same semantics as `aelf lock` with no `--for`. Mutations verified red: revert the hook fix 1 failed, 36 passed revert the review fix 1 failed, 36 passed clear every expiry in the 3 failed, 34 passed sweep (the opposite defect) The third is the control: without it both regression tests would pass against an implementation that dropped every lock_expires_at at open, which would silently make every time-boxed lock permanent. The new tests assert after a reopen rather than on the in-memory object, because the in-memory read is exactly what was already green.
Review — one defect found and fixed on the branch (
|
| mutation | result |
|---|---|
| revert the hook fix | 1 failed, 36 passed |
| revert the review fix | 1 failed, 36 passed |
| clear every expiry in the sweep | 3 failed, 34 passed |
The third is the control. Without it both new tests would pass against an implementation that dropped every lock_expires_at at open — which would silently make every time-boxed lock permanent, the opposite defect and equally invisible. It also catches your existing test_sweep_preserves_origin_expiry_and_locked_at, which is the right neighbour for it to collide with.
Smaller notes, no action taken
sweep_expired_locksdoes not filtervalid_to IS NULLwhilelist_expiring_locksdoes. So a retired belief's window is swept but never shown as expiring, and restoring it later returns it unlocked. That is arguably the correct semantics — the window did close — but the two queries disagree about whether retired rows are in scope, and only one of them says so.- The
--for/--untilmutual exclusion, calendar arithmetic (1mofrom Jan 31 → Feb 28,1yfrom Feb 29 → Feb 28), zero-window and past---untilrejections all behave as described; I re-ran those paths rather than reading them. - Your read on the two contention failures matches mine: I have hit the same pair on three branches today and they pass in isolation. That is test: subprocess-driven tests inherit the 5s global timeout with no override, so contention reports as a hang (class of #1288) #1307 and it is unrelated to this diff.
Verification
Full suite on the branch with the fix: 7,055 passed, 69 skipped, 71 xfailed. The two ..._when_fastmcp_missing failures are a local dependency-state artifact of the mcp bump and are green in CI. Branch is FF on main. Discretion grep on added lines: clean. Commit signed.
Not labelling ready-to-merge yet — waiting for CI on a71ade89.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged a71ade8 → |
|
[release:review:Setr:2026-08-04T06:06:57Z] |
1 participant
Closes #1314.
Time-boxed locks:
aelf lock "<statement>" --for 7d(alsow,mo,y,forever) and--until <date|timestamp>, mirrored on MCP asaelf_lock(expires_in=).What the semantics actually are
The issue is explicit that "after the window it decays like an unlocked belief" is the right intuition and the wrong description, and this implements the description rather than the intuition.
scoring.decay()has zero production call sites; nothing automatically moves a posterior. An expired lock stops being injected unconditionally and re-enters ordinary ranked retrieval, whereretrieval._apply_temporal_decayalready discounts it by2**(-age/half_life)fromcreated_at. A seven-day-old travel fact therefore re-enters already down-weighted. No new decay machinery, no decay-clock-origin field, anddecay()stays unresurrected.Expiry is a sweep, not a predicate
An idempotent
UPDATEat store open flips due rows tolock_level='none', after which every existinglock_levelread is correct unchanged. The rejected alternative — anow-aware term on the lock predicates — is unsafe rather than merely tedious:list_speculative_beliefsselectslock_level = 'none'and is the complement oflist_locked_beliefs, so the term applied to one and not the other drops an expired lock out of both tiers and makes it invisible to L0 and L1 alike.test_expired_lock_appears_in_both_tiers_correctlyasserts presence and absence, because absence alone is equally satisfied by a deleted belief.Three properties are load-bearing:
originis untouched. The belief was user-asserted; only its injection privilege expired. Rewriting it would reproduce the autolock origin-laundering defect.lock_expires_atandlocked_atare retained after the flip, as the trace of why the belief is now unlocked. A non-NULL expiry therefore does not imply "still locked" — which is also why the sweep'slock_level = 'user'term is load-bearing, and why a test fires when it is removed.lock:expireaudit row per flip, in the tableaelf unlockwrites to, soaelf feedshows the transition.aelf unlockclears the expiry outright. That differs from the sweep deliberately: an explicit unlock is the user revoking the lock, so the window governing it is moot, whereas in the sweep the window is the explanation.The index is not optional, and the issue asked for the measurement
The issue says to add an index only if the measurement says so. It does:
SCAN beliefs— 2.18 ms medianSEARCH … USING INDEX— 0.001 ms medianThis runs on every open of a latency-gated path the hook opens several times per turn, so 2.18 ms/open is real. The index is partial on the same
IS NOT NULLterm the query states verbatim, which is what lets SQLite match it, and keeps it tiny — permanent locks are NULL and never enter it. With nothing due the sweep is one index probe and takes no write lock. Store open measures 0.60 ms median on the branch, unchanged.The sweep runs under
_run_guarded_migration: a failing sweep leaves a lock injected one session too long, which beats a store that will not open (#1161).Parsing
Windows resolve to an absolute UTC instant at write time — a relative expression stored as-is would silently re-anchor to whenever it was next read. The parse happens before the store opens, so a typo cannot leave a permanent lock behind, which is the exact outcome the user was avoiding by passing a window.
Calendar units use calendar arithmetic:
1mofrom January 31 is February 28,1yfrom February 29 is February 28. There is deliberately no barem— it reads as either minutes or months. A past--untiland a zero-length--forare both rejected rather than accepted-and-swept.Re-locking with
--forrefreshes. Re-locking without one clears the expiry and says so — the single genuinely ambiguous case, decided on the issue so this did not have to guess.Visibility
The failure mode is a lock silently vanishing at the moment it mattered.
aelf doctorreports both halves and neither substitutes for the other: locks closing within 7 days (the chance to extend before they go) and the last sweep that fired (the only place an expiry that already happened is visible at all). Informational — a lock reaching the end of a window the user chose is the mechanism working.Verification
lock_level='user'term, clearingoriginin the sweep, approximating a month as 30 days, accepting a past--until, and applying the window only on the new-lock path each turn at least one test red.lock_expires_at IS NULLandlist_locked_beliefs()output is unchanged.github/main, discretion grep clean on added lines.Two failures appeared under full-suite contention only —
test_cli_setup_opt_out_sync.pyandtest_setup_prune_stale.py, bothTimeout (>5.0s)atselectors.py:398. Both files are untouched by this diff, both pass in isolation (21 passed, 9.1s), and store open is unchanged at 0.60 ms, so nothing here moved them. That is #1307 — subprocess-driven tests inheriting the 5s default and misreporting contention as a hang. I have now hit it on three unrelated branches today.One correction to the issue
The spec names the complement
list_unlocked_beliefs()atstore.py:4160. No such method exists. The argument is entirely sound againstlist_speculative_beliefs(), which is what actually selectslock_level = 'none', and the design is unchanged — but the reference is wrong and the code says so where it matters.Not here
Natural-language capture is carved out to #1315 as the issue directs; it needs directive detection still below its precision bar, and this deterministic substrate had to land and be testable first. Also out: resurrecting
scoring.decay(), per-belief post-expiry half-lives, expiry inference, and reminder scheduling.Rollback
Drop the sweep call site. The column goes inert, every lock reverts to permanent. No data loss, no down-migration.
Summary by Sourcery
Add support for time-boxed user locks that automatically expire and revert to ordinary retrieval, with shared parsing, CLI/MCP surfaces, and store-level sweep and observability.
New Features:
--for,--until) and MCPexpires_in, storing an absolute UTC expiry per belief.aelf locked, MCPtool_locked, andaelf doctorso users can see upcoming expiries and the last sweep that fired.Enhancements:
lock_expires_atfield and a partial index, plus an open-time sweep that flips expired locks back to unlocked while preserving audit trace.lock_expiryutility for parsing and formatting lock windows with calendar-correct arithmetic and robust error handling.Tests: