Skip to content

fix(desktop): stop agent config reverts from stale-store laundering - #3752

Closed
wpfleger96 wants to merge 17 commits into
mainfrom
duncan/agent-config-revert-fix
Closed

fix(desktop): stop agent config reverts from stale-store laundering#3752
wpfleger96 wants to merge 17 commits into
mainfrom
duncan/agent-config-revert-fix

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Jul 30, 2026

Copy link
Copy Markdown
Member

Agent system prompts, models, and thinking/effort settings revert silently when more than one Buzz build shares an identity against the same relay. Agent configs sync as NIP-33 replaceable events (kind:30175/30176/30177) resolved last-write-wins on created_at, and every desktop boot re-signs whatever its local store holds at monotonic_created_at = max(now, head + 1). A stale store therefore launders old content into a winning head at every launch.

This PR fixes both halves: the ordering primitive that cleans up the retention cache, and the boot decision pass that stops the laundering before it can publish.

Part 1 — Ordering

Pending local intent outranks ordering

pending_sync = 1 marks durable local intent — an edit the user made that this device has not published yet. retain_inbound_event now checks for a pending row before any timestamp or event-id comparison, and returns a distinct Deferred outcome that mutates nothing.

A newer created_at is not evidence of newer intent. The laundering vector mints events at monotonic_created_at = max(now, head + 1), so every laundered revert arrives strictly newer than the edit it reverts. Resolving inbound events by timestamp alone therefore handed the win to precisely the untrusted input: a strictly newer event cleared pending_sync and patched personas.json, destroying the unpublished edit before any arbitration could see it.

Callers in commands/personas/inbound.rs gate on Applied, so neither non-apply outcome reaches disk. Deferred stays distinct from Skipped so the decision pass can tell "lost the ordering compare" from "still owes arbitration."

The ordering bug

For non-pending rows, the retention cache and the relay disagreed about which of two same-second events is the head.

replace_parameterized_event (crates/buzz-db/src/lib.rs) breaks an equal created_at by lowest event id — it rejects an incoming event when created_at == accepted_ts && incoming_id >= accepted_id. The desktop cache skipped every equal-timestamp inbound event instead. Nostr timestamps are seconds-granularity, so this collision is reachable in normal use, and once the cache held a head the relay disagreed with, every later disk-vs-head comparison inherited the error.

retain_inbound_event now applies the relay's own rule to non-pending rows.

Trusting the ordering key

event_id is stored as a real column rather than reparsed from raw_event on every compare, and it is always derived from an event that parsed and cryptographically verified — never read from the JSON's self-asserted id field. A legacy row whose stored bytes do not verify keeps event_id IS NULL and is treated as unorderable: the retained row stands and nothing is decided from a fabricated key.

RetainedEvent::pending / ::inbound replace the hand-built struct literals at all 13 call sites. Every event-derived field is read from the single event passed in, so a row built through them cannot carry an event_id describing different bytes than its own raw_event.

Schema migration

baseline_event_id and baseline_content are added alongside event_id; they carry the provenance the decision pass reads. The migration is additive and crash-safe. A cheap read-only pragma_table_info probe decides whether anything is missing; only then does it open a BEGIN EXCLUSIVE transaction, re-probe inside the lock, and apply the ALTER TABLEs together with the event_id backfill. Two processes opening the same database concurrently are safe — the loser waits on the write lock, re-probes, and finds nothing to do.

Part 2 — Boot decision pass

Root cause

The laundering vector exists because boot reconcile re-signs the stale store's content at monotonic_created_at, minting events that are definitionally newer than the edits they revert. created_at cannot arbitrate this: the value is manufactured by the reconcile, not observed. No ordering rule can fix it.

The barrier arbitrates on provenance instead. A baseline records what this install last agreed was published at a coordinate. With no baseline, the install cannot show it ever agreed to anything there, so neither its content nor its timestamps are evidence of intent about the relay's state — the pending row must be withheld until the user resolves the conflict or a future boot with a baseline can arbitrate cleanly.

Decision table (decide)

Four ordered gates:

Gate Condition Outcome
A Positive tombstone evidence Forward to deletion arbitration
B No baseline, live head present Park — withhold, surface conflict
C Queued deletion vs head timestamp PublishDeletion or Defer
D Queued edit vs baseline projection Publish, StampBaseline, or Defer

Gate ordering is load-bearing: gate C's timestamp compare is sound only because gate B already excluded every store that would win it spuriously (the manufactured-timestamp path).

Both publication gates are enforced independently, because a coordinate's live record and its queued tombstone occupy different primary keys and the flush loop reads them independently. The live gate is default-deny over the decision set, so a variant added later is withheld until deliberately admitted.

Evidence

  • Head lookups — exact per-coordinate {kinds, authors, #d, limit:2} queries. A transport failure becomes LookupFailed, never Absent, so a flaky network cannot masquerade as a deletion.
  • Tombstone scan — one per-owner paginated kind:5 walk. #a is post-filtered server-side, so absence is claimed only from a page shorter than the limit. The scan runs once per boot and yields evidence for all coordinates in one pass.

Composed regression (Will's bug)

test_stale_store_with_no_baseline_cannot_revert_a_newer_head and test_no_baseline_store_withholds_both_the_record_and_its_tombstone exercise the exact scenario: a second install, no baseline, stale disk, live head. Gate B parks it; the flush loop suppresses both the live record and its tombstone row.

The composed regression path runs through the real run_decision_pass/gate enforcement path, not a mock, verifying the full decide → apply_gate → get_pending_sync composition.

Known limitations

On relay deployments that route historical reads through a read-replica pool, head lookups and the tombstone scan read best-available rather than writer-consistent. A replica that has not yet replayed a write answers absence in the dangerous direction. The exposure window equals replica lag (normally single-digit seconds) and requires the coincidence of an agent-config write followed immediately by a boot-time barrier read from behind the lag. The barrier's per-decision tracing line surfaces this if it fires in the wild. Re-adding a writer-pinned read opt-in to the protocol is a small standalone relay PR reversible on observed evidence.

Deferred

Explicit conflict resolution surface (Step 6 / V3.6) — follow-up work, not in this PR: a conflict UI backed by a get_blocked_coordinates query and push-local / accept-remote commands.

A coordinate parked by the barrier (publish_blocked = 1) stays parked for the remainder of the session unless the user actively edits it: every user-initiated save or tombstone calls retain_user_intent_event, which clears publish_blocked so the edit publishes in-session. An edit-free session on an install that was parked at boot will not publish that coordinate until the resolution surface ships. This is the correct polarity for dev-build collisions (the scenario this PR closes) — a stale build that takes no explicit action stays silent. It also affects a legitimate second install where the user has not yet made an edit on that machine; the workaround there is to re-save the agent in the app, which clears the gate and publishes the local version.

The support path for silent parks is the barrier's per-decision tracing line (buzz::config_sync), which logs every Park decision with coordinate, kind, and reason, and direct SQL (UPDATE persona_events SET publish_blocked = 0) against the retention DB. The follow-up delivers a conflict UI with push-local / accept-remote actions, clearing publish_blocked and stamping a baseline atomically per coordinate.

@wpfleger96
wpfleger96 requested a review from a team as a code owner July 30, 2026 14:39
@wpfleger96
wpfleger96 force-pushed the duncan/agent-config-revert-fix branch from b29f86f to cd8f244 Compare July 30, 2026 22:19
@wpfleger96 wpfleger96 changed the title fix(desktop): order retained agent events by the relay's own comparator fix(desktop): stop agent config reverts from stale-store laundering Jul 30, 2026
npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 and others added 14 commits August 1, 2026 14:09
The retention cache resolved an equal-`created_at` collision by skipping the
inbound event unconditionally, while the relay breaks that tie by lowest event
id. The two could therefore disagree about which same-second event is the head,
and every later disk-vs-head compare inherited the error.

Retained rows now carry the event id as a real column so the local compare can
match `replace_parameterized_event`. A pending row still wins its tie
regardless of id: it is durable local intent, arbitrated against a
writer-consistent head by the boot pass rather than dropped by a cache compare.

The id is re-derived from a parsed and verified event, never read from the
JSON's self-asserted `id` field, so a legacy row whose bytes do not verify
stays unresolved instead of receiving a key it could win a tie with.

`RetainedEvent::pending`/`::inbound` replace the hand-built literals at every
call site, which makes it structurally impossible for a row's ordering key to
describe different bytes than its own `raw_event`.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A pending row is durable local intent — an edit this device made and has
not yet published. Resolving inbound events by timestamp let a strictly
newer event clear `pending_sync` and patch `personas.json`, destroying
that edit. Timestamps are not evidence of newer intent: the laundering
vector this change targets re-signs stale content at
`max(now, head + 1)`, so every laundered revert arrives strictly newer.
That made the untrusted input the one that always won.

The pending check now precedes every ordering rule, and the new
`Deferred` outcome keeps "lost the compare" distinct from "awaiting
arbitration" for the boot decision pass. Callers gate on `Applied` so
neither non-apply outcome reaches disk. Until that pass lands a pending
row shadows genuinely newer remote edits for its coordinate; flush
normally clears it within seconds, and an edit that can still be
reconciled is worth more than one that cannot be recovered.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Boot reconcile re-signs whatever is on disk at `monotonic_created_at =
max(now, head + 1)`, so a merely-stale store mints events strictly newer
than the edits they revert. Last-write-wins then adopts the revert. A
second install with a fresh retention database is exactly this state,
which is why a dev build silently reverts config edited elsewhere.

`created_at` cannot arbitrate it: the value the stale store carries is
manufactured by the reconcile, not observed. The barrier arbitrates on
PROVENANCE instead. A baseline records what this install last agreed was
published at a coordinate; with no baseline, the install cannot show it
ever agreed to anything there, so neither its content nor its timestamps
are evidence of intent about the relay's state.

`decide` runs four ordered gates: positive tombstone evidence, then the
no-baseline park, then the queued-deletion timestamp compare, then the
queued edit against the baseline. The ordering is load-bearing — the
timestamp compare is sound only because the no-baseline gate already
excluded every store that would win it spuriously.

Both publication gates are enforced, because a coordinate's record and
its queued tombstone occupy different primary keys and the flush loop
reads them independently: gating one row computes a suppression and
enforces half of it. The live gate is default-deny over the decision
set, so a variant added later is withheld until deliberately admitted.

The scan is per-owner and unfiltered by `#a` because the relay
post-filters `#a` after its `LIMIT`, so absence is claimed only from a
page shorter than the limit it asked for.

Nothing is applied to disk here. `ApplyHead`, `RestoreFromRelay`, and
`DeleteLocal` are decided, logged, and gated; resolving them is the
user-driven step.

On relay deployments with a replica-routed read pool, the head lookups
and tombstone scan read best-available rather than writer-consistent;
staleness within replication lag is a documented known limitation (see
`head_lookup` and `tombstone_scan` module docs). The barrier's
per-decision tracing line is the detection mechanism if it fires.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Add a per-scope readiness latch (config_sync_ready_scope in AppState)
that the flush loop checks before publishing any row. Before this, the
flush loop ran independently of the boot barrier, so legacy pending rows
from a prior boot were publishable at t≈0 — before reconcile or barrier
had run — recreating the stale-store revert this PR exists to close.

Changes:

- Add publish_blocked field to RetainedEvent and carry it in all query
  sites (get_pending_sync, get_retained_event, get_retained_personas,
  legacy_migration reader). New rows start publish_blocked: false;
  the barrier sets it via set_publish_blocked.

- Add config_sync_ready_scope: Mutex<Option<PathBuf>> to AppState.
  Starts None. Set to Some(db_path) by run_boot_barrier on successful
  completion; never set on any error path.

- flush_active_pending_events checks readiness before snapshotting
  pending rows. If not ready it runs run_boot_barrier inline, then
  re-checks: a barrier failure skips this tick and retries on the next,
  so a transient relay-unreachable at boot does not wedge publishing
  for the session.

- flush_pending_events_at re-reads publish_blocked immediately before
  submit_signed_event_at_with_keys. A row gated after the pending
  snapshot was taken cannot publish.

- apply_workspace clears config_sync_ready_scope to None before
  spawn_event_sync so a workspace switch lands unready until its own
  barrier passes.

- Remove the 'in practice' timer-luck paragraph from event_sync.rs;
  replace with the enforced structural invariant.

- Remove all writer-consistent and sync_authoritative references from
  desktop/src-tauri; replace with exact, best-available,
  replica-lag-exposed language. Grep acceptance: empty.

- Add three deterministic tests in flush_barrier module:
  (a) test_row_gated_after_snapshot_cannot_submit: barrier gates a row
      after the pending snapshot; flush sees publish_blocked on re-read
      and skips it; row stays pending.
  (b) test_barrier_error_leaves_scope_unready: latch starts None, stays
      None until barrier succeeds, cleared on workspace switch.
  (c) test_retry_path_publishes_after_barrier_succeeds: latch None →
      scope not ready; latch set → flush proceeds and row publishes.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Replace map_or(true, ...) with is_none_or(...) per clippy lint
unnecessary_map_or (-D warnings gate).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…pth under latch

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…h barrier

spawn_event_sync previously called mark_unready() between the reconcile
and its trailing run_boot_barrier call, opening a window where the flush
loop could win the claim_in_progress CAS and certify readiness against
the pre-reconcile database state. A stale row retained by reconcile would
then publish before the post-reconcile barrier could gate it.

Fix: hold InProgress continuously through the entire reconcile+barrier
window. Add run_boot_barrier_after_claim (skips the CAS) called by
spawn_event_sync; run_boot_barrier (with CAS) remains the flush-loop
entry. The process-global static in config_sync_readiness.rs makes the
latch app-state-free, keeping app_state.rs within its size gate.

Also split flush-barrier tests into a dedicated file and rewrite them
to exercise production transition functions via module-level free
functions, with thread_local isolation for test parallelism. Each test
now exercises a named production invariant: (a) snapshot-vs-gate race,
(b) InProgress rejects second claim, (c) barrier error resets to Unready,
(d) Thufir's deterministic interleaving proof, (e) retry publishes.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…I panic recovery

apply_workspace previously called mark_unready() then relied on
spawn_event_sync's inner claim_in_progress() CAS to reclaim InProgress
before migration and reconcile ran. The flush loop (alive since process
start) could win that CAS in the window between mark_unready() and the
spawn, certifying readiness against pre-migration, pre-reconcile DB state.
Two consequences: reconcile silently dropped (spawn_event_sync returns
early on failed CAS), and legacy migration rows published unarbitrated.

Fixes:

PRIMARY: apply_workspace now calls force_claim_in_progress() atomically
BEFORE migrate_legacy_retention_into. The claim (an RAII ReadinessClaim
guard) is held across the entire sequence: legacy migration -> reconcile
-> barrier -> mark_ready/Unready. spawn_event_sync_with_held_claim takes
the held claim instead of racing its own CAS. flush loop sees InProgress
throughout and cannot interleave.

SECONDARY: ReadinessClaim RAII guard replaces the manual mark_unready/
mark_ready calls at barrier entry/exit sites. On success, claim.resolve()
prevents the drop from resetting to Unready; on error or panic, the drop
resets to Unready automatically. A panic inside run_boot_barrier_for_scope
no longer leaves the latch permanently wedged at InProgress.

API changes:
- claim_in_progress() returns Option<ReadinessClaim> (was bool)
- force_claim_in_progress() added: preempting, any-state -> InProgress
- ReadinessClaim: RAII guard with resolve() explicit success path
- run_boot_barrier_after_claim() takes ReadinessClaim (transfers ownership)
- spawn_event_sync removed (dead code); spawn_event_sync_with_held_claim
  is now the sole spawn entry point

New test (f): apply_workspace window -- force_claim preempts any state;
flush claim between invalidation and reconcile is rejected; migrated and
reconciled rows gated by post-reconcile barrier cannot publish.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ent stale-claim mutations

A preempted ReadinessClaim had no ownership identity: after
force_claim_in_progress() seized the latch from a flush-owned
InProgress, the flush's in-flight barrier could still call
complete()/drop and unconditionally mutate the latch. Two legs:

- Leg 1 (same-scope case): stale flush barrier finishes, calls
  resolve()+mark_ready() → latch certified Ready while reconcile
  was still retaining rows → stale rows publish unarbitrated.
- Leg 2: stale barrier errors → RAII drop resets force-claim's
  InProgress to Unready mid-migration → next flush tick wins a
  fresh claim and certifies against mid-sequence state.
- Leg 3: run_boot_barrier_for_scope's abandon arm returned Ok(())
  which the enforcing wrapper treated as Success.

Fix: the latch carries a u64 generation counter. force_claim_in_progress
increments it; claim_in_progress records the current generation.
Latch mutations move onto the claim:

- claim.complete(db_path) → Ready only if latch generation matches.
- Drop → Unready only if generation matches.
- Preempted claims are silent no-ops in both paths.

BarrierOutcome enum added; abandon arm returns Abandoned (not
Success) so run_boot_barrier_enforcing never calls complete() for
an abandoned scope. mark_ready() and resolve() removed from the
public API; mark_unready() gated to #[cfg(test)] since all
production transitions now go through the claim.

Tests (g)/(h): stale complete/drop are no-ops after force-claim.
Test (i): leg-1 end-to-end interleaving — stale barrier complete
cannot certify Ready; rows retained after the stale complete
cannot publish until force-claim's own barrier passes.
Tests (c)/(e)/(f) updated to claim.complete() API.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…rrier, reconcile Result propagation

Four pass-3 findings resolved:

1. CRITICAL — generation-gate the enforcement phase: added `is_current()` on
   `ReadinessClaim`; `run_boot_barrier_for_scope` now checks it while holding
   `managed_agents_store_lock` and returns `Abandoned` without writing if the
   claim is preempted. Stale relay evidence (e.g. `HeadState::Absent` re-opening
   a parked coordinate) can no longer reach `run_decision_pass`.

2. IMPORTANT — test (j): exercises the stale barrier's actual decision pass
   through the enforcement seam. Vacuity block confirms the stale evidence WOULD
   clear `publish_blocked` on an unguarded scratch db; enforcement seam shows
   the guard blocks the write on the real store.

3. IMPORTANT — `import_identity` now calls `force_claim_in_progress` and
   `spawn_event_sync_with_held_claim` after swapping keys, so the new owner's
   disk projections are enqueued and the barrier certifies the scope ready.
   Without this, config publication stopped silently until restart.

4. IMPORTANT — reconcile legs return `Result`: `migrate_personas_to_events`,
   `migrate_teams_to_events`, and `reconcile_agents_to_events` all return
   `Result<(), String>`; `run_event_sync` propagates with `?`;
   `spawn_event_sync_with_held_claim` drops the claim without completing when
   any leg fails, leaving the scope `Unready` for retry rather than certifying
   `Ready` after a partial reconcile.

All four fixes are in `desktop/src-tauri` only. 2111 tests pass, clippy -D warnings
clean, cargo fmt clean, just desktop-check green.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ce_decision_pass seam

Fix 1 (BLOCKING): the flush loop's Unready arm previously called
run_boot_barrier (barrier-only), which certifies Ready without running
reconcile. If a prior reconcile leg failed — leaving the scope Unready —
the barrier-only retry would strand that leg's disk edits for the
session (fail-forever). Fix: replace run_boot_barrier with
claim_in_progress → spawn_event_sync_with_held_claim, which runs the
full reconcile+barrier sequence. run_boot_barrier is removed (now dead);
test: reconcile_failure_retry_with_full_transition_certifies_and_enqueues.

Fix 2 (IMPORTANT): test (j)'s stale-barrier enforcement seam was a
mirrored conditional — it replicated if stale_claim.is_current()
manually rather than driving the production guard. Deleting
is_current() from config_barrier.rs left the test green. Extract
enforce_decision_pass(claim, conn, owner_pubkey, states) from
run_boot_barrier_for_scope Phase 4; drive test (j) through it. Now
removing is_current() from enforce_decision_pass fails the test.

Fix 3 (MINOR): correct the rationale comment in import_identity. The
prior comment claimed the scope database does not change on key swap;
scoped_retention_db_path hashes the owner pubkey so it does. Update to
explain why skipping migrate_legacy_retention_into is still defensible
(stranded legacy row adopted on the next boot's apply_workspace;
consequence bounded to the import session).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… errors

IMPORTANT#1: move force_claim_in_progress() before active_retention_scope()
in both workspace.rs and identity.rs. On scope-resolution failure the claim
now drops via RAII → Unready, so the flush loop can retry. Previously a
transient scope failure after a key/relay swap left the stale Ready(old_path)
latch alive; claim_in_progress rejects from any Ready state, so publication
was permanently stopped for the session.

New test: test_scope_resolution_failure_after_force_claim_leaves_latch_unready_and_retryable
covers Ready(A) → swap → scope failure → Unready → retry claims and runs the
full transition for scope B.

IMPORTANT#2: replace let Ok(base_dir) = managed_agents_base_dir(app) else { return Ok(()) }
with let base_dir = managed_agents_base_dir(app)? in all three reconcile
wrappers (migrate_personas_to_events, migrate_teams_to_events,
reconcile_agents_to_events). Base-dir failures now propagate through
run_event_sync's Result; a failed leg cannot silently certify Ready.

New test: test_base_dir_failure_propagates_through_run_event_sync drives the
new run_event_sync_in_dir seam with a bad db_path, asserts Err returns and
scope stays Unready, then proves the full retry with a valid path enqueues
the row and certifies Ready.

MINOR: rewrite config_barrier.rs enforcement comment to state the real
invariant — managed_agents_store_lock does NOT freeze the readiness
generation; force_claim_in_progress takes only the readiness mutex. A
preempted force-claim sets InProgress on a new generation and its own
barrier (or a full flush retry) supplies the final gate writes.

Prose nits: fix flush_barrier_tests.rs:216 reference to deleted
run_boot_barrier; fix identity.rs comment that referenced is_ready_for
(now #[cfg(test)]) — flush uses readiness_state() directly.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ction seams

IMPORTANT#1: extract force_claim_and_resolve() helper in config_sync_readiness.rs
- Both apply_workspace and import_identity route through force_claim_and_resolve()
  rather than calling force_claim_in_progress() + active_retention_scope() inline.
- On resolver failure the helper drops the claim (RAII -> Unready); no stale
  Ready(old_path) survives.
- Test drives force_claim_and_resolve() with a failing resolver; reordering the
  helper (resolve before claim) leaves Ready(A) intact and fails the assertion.

IMPORTANT#2: factor run_event_sync through run_event_sync_impl with injectable resolver
- run_event_sync_impl<F: FnOnce() -> Result<PathBuf, String>>(...) is the shared
  non-test core; the single base_dir_fn()? is the mutation-sensitive propagation
  point that replaced the three per-wrapper let-Ok-else-return-Ok swallows.
- run_event_sync() passes managed_agents_base_dir(app) as the resolver.
- reconcile_agents_to_events() removed (dead; reconcile_agents_in_dir_at promoted
  to pub(crate) for direct use by the impl core).
- Test injects Err via run_event_sync_impl(|| Err(...)); swapping ? to swallow
  returns Ok(()) and fails result.is_err().

MINOR: rewrite enforce_decision_pass() function-level contract doc
- Replaces false store-lock-prevents-generation-change claim with the real
  invariant: store lock serializes observation/writes; post-check force-claim sets
  InProgress on the new generation and its own barrier supplies authoritative writes.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…nt gate, uncertain-lookup barrier, single publisher

Addresses all five blocking findings from am's and peon's reviews:

am#1 / peon-P1#1: Move baseline provenance to a dedicated persona_baselines
table keyed by coordinate. The old in-row columns were wiped on delete, causing
an offline delete to park forever. The new table survives delete_retained_event,
so the next boot sees baseline=Present + queued deletion → Gate C arbitration.
stamp_baseline is now written at both boot (StampBaseline) and publish-confirm
(mark_synced_and_stamp_baseline in flush_pending_events_at), killing the
one-edit-later revert vector: after publishing v2, the baseline records v2, so
a later boot with head=v3 sees queued==baseline and stays silent.

am#2 / peon-P2#1: Separate user-intent writes from reconcile writes at the
retention API. retain_user_intent_event clears publish_blocked unconditionally;
retain_event (reconcile path) never touches the gate. All user-facing save and
tombstone paths now call retain_user_intent_event, so an edit to a parked
coordinate takes effect in-session without waiting for the next boot.

peon-P1#3: A decision pass driven by LookupFailed or ScanIncomplete must not
certify the scope Ready. run_decision_pass now returns a has_uncertain_gates
flag; enforce_decision_pass returns Abandoned when it is set, dropping the
claim and leaving the latch Unready for the 30s flush retry. A wifi-race boot
costs seconds, not a whole muted session. The false 'next pass' comment in
decision.rs is corrected.

peon-P2#2: set_persona_shared and update_persona_and_publish no longer submit
directly to the relay. Both route through publish_prepared_persona_via_flush,
which calls retain_user_intent_event (gate cleared for user intent) then
flush_pending_events_at (per-row publish_blocked re-read). The duplicated
publish/mark-synced logic in sharing.rs is deleted; retention.rs:508's
invariant is now total.

P3 (non-blocking): Deferred paragraph in PR body updated to reflect the
sharpened stakes of parked rows.

Regression tests (a)-(e): one test per finding, each traversing the production
seam it protects.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 force-pushed the duncan/agent-config-revert-fix branch from 2c37a75 to 4c2c845 Compare August 1, 2026 18:16
npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 and others added 3 commits August 1, 2026 15:23
…, atomic mark-synced, baseline join

Four correctness fixes on top of the am/peon feedback round:

1. publish_prepared_persona_via_flush now checks the readiness latch
   (ReadinessState::Ready for exactly the prepared scope's db_path) before
   calling flush_pending_events_at. Unready or InProgress → row stays
   pending, returns Queued. Avoids publishing during boot/workspace
   reconcile window before scope-wide arbitration completes.
   New test: test_publish_via_flush_blocked_when_latch_not_ready uses an
   accepting relay so a bypass would flip the assertion.

2. retain_agent_record gains a user_intent: bool parameter. Interactive
   saves pass true (clears publish_blocked even on unchanged-content
   early-return); boot reconcile passes false. tombstone_managed_agent_pending
   and tombstone_team_pending route through retain_user_intent_event.
   snapshot import callers (team_snapshot, personas/snapshot/import) use
   retain_user_intent_event. All three config kinds now reopen gated rows
   on explicit user action.
   New tests: test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not
   (via retain_agent_record production seam) and
   test_non_persona_tombstone_user_intent_clears_gate (kind-5 tombstone path).

3. mark_synced_and_stamp_baseline wraps both SQL statements in a single
   SQLite transaction (unchecked_transaction). The baseline is only stamped
   when the compare-and-clear affected the intended row (rows_changed == 1).
   A crash or error between the two statements can no longer produce
   pending_sync=0 without a baseline stamp.
   New test: test_mark_synced_and_stamp_baseline_atomic_both_written_on_success
   verifies both pending_sync cleared AND baseline stamped in one call.

4. quarantine_unprovable_pending replaces the stale
   baseline_event_id IS NULL column check with a NOT EXISTS subquery
   against persona_baselines (the table baselines now live in).
   publish_prepared_persona_via_flush replaces the aggregate-flushed-count
   + row-absence published heuristic with a per-coordinate check: baseline
   stamped with the expected event_id (primary) OR row pending_sync cleared
   with matching content (fallback).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Three test-correctness fixes, no production logic changes:

1. test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not:
   The interactive-save call previously used the same record (unchanged
   content), so the early-return branch fired and set_publish_blocked(false)
   cleared the gate before the retain_user_intent_event path was reached.
   Reverting the changed-content branch to retain_event left the test green.
   Added Case B: interactive save with a different system_prompt, which
   exercises the retain_user_intent_event path directly. Also added
   Case D: reconcile with changed content, confirming retain_event does
   not clear the gate. Now all four combinations (same/changed × user/reconcile)
   are verified.

2. test_non_persona_tombstone_user_intent_clears_gate:
   Previously called retain_user_intent_event directly — reverting
   tombstone_managed_agent_pending to retain_event left zero test callers
   on the production path, so the suite stayed green. Extracted
   tombstone_agent_pending_inner from commands/agents.rs into
   managed_agents/reconcile.rs (pub(crate), symmetric with retain_agent_record)
   and rewired tombstone_managed_agent_pending to call it. Test now drives
   the seam directly. Pre-seeds the tombstone coordinate with
   publish_blocked=true so retain_event (which leaves the existing value)
   and retain_user_intent_event (which calls set_publish_blocked(false))
   produce distinguishable outcomes. Mutation at the production call site
   now fails the suite.

3. test_mark_synced_and_stamp_baseline_atomic_both_written_on_success:
   Previously tested the success path only. Added Case B: drops the
   persona_baselines table before the call, forces the baseline INSERT to
   fail, and asserts the call errors AND pending_sync is still 1
   (transaction rolled back). If the function is split back into two
   autocommit statements, the UPDATE commits first and pending_sync becomes 0
   before the INSERT fails — the rollback assertion catches the regression.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Targets 2 and 3 of Paul's consolidation dispatch:

- decision/tests.rs: −58 lines (5 row tests fully implied by property
  sweeps removed: LookupFailed duplicates, and 3 small cells already
  provable from the sweeps' 972-state coverage)
- config_sync/tests.rs: −443 lines (10 fine-grained unit tests removed
  whose behavior is fully exercised by the composed regression scenarios;
  test scaffolding extracted to shared test_helpers.rs)
- config_sync/regression_tests.rs: −101 lines (tests for the same-content
  early-return path and other coverage fully subsumed by the composition
  tests)
- config_sync/test_helpers.rs: +112 lines (new shared scaffolding used by
  all four test files)

Target 1 (retention/tests.rs relocation) is blocked: inlining 471 lines
of test code back into retention.rs would push the file from 959 to ~1430
lines, which violates the desktop file-size ratchet (1000-line limit).
Reported to Paul as a blocker.

No production-logic changes. Non-test file changes: config_sync.rs adds
the test_helpers module declaration (#[cfg(test)] only).

All four mutation checks still catch their reversions:
- Mutation 1 (retain_agent_record → retain_event): FAILS
  test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not
- Mutation 2 (tombstone_agent_pending_inner → retain_event): FAILS
  test_non_persona_tombstone_user_intent_clears_gate
- Mutation 3 (mark_synced_and_stamp_baseline split to autocommit): FAILS
  test_mark_synced_and_stamp_baseline_atomic_both_written_on_success
- Mutation 4 (readiness check removal): FAILS
  test_publish_via_flush_blocked_when_latch_not_ready

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96

Copy link
Copy Markdown
Member Author

🤖 Closing without merge — direction change, not a defect.

This client-side protection is superseded by the relay-side migration in #4593 and its follow-up slices: relay-enforced predecessor CAS stops the timestamp-laundering resurrection at admission, including from stale binaries — the one property no client-side design can have. The detailed use cases, surface inventory, and regression scenarios from this work have been handed off to that effort.

The branch duncan/agent-config-revert-fix is retained as the reference implementation (review-complete, CI green at tip). If the relay rails slip or the reverts become intolerable in the interim, this reopens as-is.

@wpfleger96 wpfleger96 closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant