fix(exploration): clamp the fire-index claim to the ledger high-water mark

[robotrocketscience]

Closes #1308.

MemoryStore.next_exploration_fire_idx read the counter out of schema_meta, and on a parse failure fell back to current = 0 before nxt = current + 1 — so a corrupt value returned index 1 regardless of what the ledger already held, re-issuing every index the lane had already used and breaking the never-decrease invariant #1294 established.

The fix is an unconditional clamp, not a corrupt-value branch

The obvious shape — trigger the recovery only when the value fails to parse, or when it is negative — does not meet the issue's own first acceptance bullet. A hand-edited smaller positive value is both the likeliest corruption and invisible to that trigger: schema_meta = '5' with ledger rows at 20/40/60 parses fine, is not negative, and yields nxt = 6, re-issuing 6..60.

So the claim is unconditional:

current = max(current_from_schema_meta, ledger_high_water)

The "keep it conditional to protect the hot path" argument does not survive inspection: the caller fires at most once per UserPromptSubmit turn, and only after the enabled check, which is default-OFF — and MAX on an indexed column is a single seek.

The pooling question, stated rather than buried

store.py's own docstring says pre-#1294 and post-#1294 exploration_events rows are not comparable and must not be pooled, the same way #1016-B partitions the injection-pack series. An unqualified SELECT MAX(fire_idx) pools across exactly that boundary.

This is called out in a code comment and in the commit message rather than passed over. The reasoning for accepting it: the clamp can only move the counter up, never down and never backwards over an already-issued index. Pooling across the era boundary can therefore only make the claim more conservative — it cannot cause the re-issue this fix exists to prevent. Reviewers who disagree should say so, because the alternative (qualifying the read by era) needs a schema distinction that does not currently exist.

Honest population note

The lane is default-OFF, and the only writer of the key is str(int), so no code path can produce a corrupt value. Reaching this branch requires a hand-edit or disk corruption. This is a cheap invariant guard, not a live-incident fix, and should be weighed as such.

Verification

Mutations verified to go red (file staged before mutating, restored after) — see the commit message for the full table. Tests were run with AELFRICE_DB pinned to a fresh temp path so the repo-local live store cannot leak in. CHANGELOG is insert-only under [Unreleased].

Summary by Sourcery

Clamp the exploration fire-index counter to the ledger high-water mark to prevent re-issuing indices already recorded in the exploration ledger.

Bug Fixes:

• Prevent corrupt or hand-edited exploration counters from restarting at 1 or a smaller value and re-issuing previously used fire indices.
• Ensure a corrupt counter on an empty exploration ledger still yields a valid starting index without wedging or raising.

Enhancements:

• Document the exploration counter clamp behaviour and its pooling across pre- and post-change regimes in the store docstring.
• Strengthen test coverage around exploration fire-index sequencing, re-arming behaviour, and the interaction between the counter and the ledger high-water mark.

Documentation:

• Add a changelog entry describing the exploration counter clamp, its safety properties, and the conditions under which it is exercised.

[sourcery-ai]

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Review limit reached

@robotrocketscience, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 57c26e86-2f36-4e8c-8d1b-d336b746ca2d

📥 Commits

Reviewing files that changed from the base of the PR and between ad206de and 050e4a6.

📒 Files selected for processing (3)

• CHANGELOG/v4.md
• src/aelfrice/store.py
• tests/test_exploration_slot_1279.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

This PR changes the exploration fire-index allocation so it is always clamped to the ledger high-water mark, adds tests to pin the non-regression and edge cases around corrupt/hand-edited counters, adjusts a test helper to keep the ledger consistent for re-arming scenarios, and documents the fix in the changelog.

Sequence diagram for clamped exploration fire index allocation

sequenceDiagram
    participant Caller
    participant Store
    participant schema_meta
    participant exploration_events

    Caller->>Store: next_exploration_fire_idx()
    Store->>schema_meta: SELECT value FROM schema_meta
    schema_meta-->>Store: value or null
    Store->>Store: parse int(value) or set current = 0
    Store->>exploration_events: SELECT MAX(fire_idx) AS m FROM exploration_events
    exploration_events-->>Store: high_water = m
    Store->>Store: current = max(current, high_water)
    Store->>Store: nxt = current + 1
    Store->>schema_meta: INSERT OR REPLACE exploration_fire_idx = nxt
    Store-->>Caller: nxt

Loading

Entity relationship diagram for exploration fire index clamping

erDiagram
    schema_meta {
        string key
        string value
    }

    exploration_events {
        int fire_idx
        int scope_id
        string query
    }

    schema_meta ||--o{ exploration_events : tracks_fire_indices
    schema_meta }o--o{ exploration_events : clamped_to_MAX_fire_idx

Loading

File-Level Changes

Change	Details	Files
Clamp the exploration fire index counter to the ledger high-water mark on every call, including the corrupt-value branch.	Modify next_exploration_fire_idx to always read MAX(fire_idx) from exploration_events and use it as a floor via max(current, ledger_high_water). Keep corrupt or unparsable schema_meta values from wedging the lane by falling back to 0 and then applying the clamp instead of restarting the sequence at 1. Extend the method docstring to document the unconditional clamp, the pooling across pre/post-#1294 regimes, and the performance/semantics rationale.	src/aelfrice/store.py
Update and extend exploration tests to validate the clamp behavior and prevent silent non-firing in re-arming scenarios.	Change the _fire helper to delete exploration_events rows with fire_idx >= idx before reseeding the counter, so tests truly exercise re-arming against an existing ledger. Add helper functions to populate ledger rows at specific indices and to set arbitrary schema_meta counter values. Add tests asserting that corrupt, hand-edited smaller, empty-ledger, and intact-counter cases all produce the expected next_exploration_fire_idx behavior without re-issuing indices.	tests/test_exploration_slot_1279.py
Document the corrupt exploration counter behavior and the new clamp semantics in the changelog.	Add a v4 changelog entry under [Unreleased] describing the previous restart-at-1 defect and the unconditional clamp fix. Explain the interaction with derive_seed, the non-decreasing index invariant from feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1176 proposal 5) #1294, and that this is an invariant guard for rare corruption rather than a fix for a live incident.	CHANGELOG/v4.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#1308	Ensure that a corrupt or hand-edited schema_meta exploration counter does not re-issue a fire_idx that already appears in exploration_events, preserving the post-change "never decrease" invariant.	✅
#1308	Add tests that fail against the old current = 0 behavior and verify: (a) corrupt/hand-edited values with existing ledger rows do not re-issue indices, and (b) the empty-ledger case still returns 1 rather than raising.	✅
#1308	Update the next_exploration_fire_idx documentation so the docstring and code agree about the "never decrease" / non-reissue invariant, including the corrupt-value branch.	✅

Possibly linked issues

• feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1176 proposal 5) #1294: PR changes next_exploration_fire_idx to clamp against MAX(fire_idx) and adds tests, fixing the re-issued index defect from feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1176 proposal 5) #1294.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[robotrocketscience]

[claim:review:Setr:2026-08-04T04:52:37Z]

[robotrocketscience]

Review — no defects found; rebased onto main (050e4a6d)

I tried to break this and could not. What I checked, and how:

The unconditional form is the right call, and the argument in the body holds up. The issue's own suggested patch puts the recovery inside the except, and the smaller-positive-hand-edit case walks straight past it. test_a_hand_edited_smaller_value_does_not_re_issue is the arm that makes that concrete, and it is the reason to prefer this shape over the one the issue proposed.

Mutation-verified independently. Deleting the five clamp lines from store.py turns test_a_corrupt_counter_does_not_re_issue_a_ledger_index and test_a_hand_edited_smaller_value_does_not_re_issue red (24 passed → 2 failed). The other two new tests stay green, which is correct — the empty-ledger and intact-counter arms are the "did you satisfy this by refusing to issue / by perturbing the healthy path" controls, and neither should be sensitive to the clamp.

Both performance claims in the docstring are true at the call site, not just plausible. idx_exploration_events_fire is ON exploration_events(fire_idx) — single column, so SELECT MAX(fire_idx) is the index-max seek you claim and not a scan. And hook.py:1697 claims the index after is_exploration_enabled(start=cwd) returns, so a default-off install genuinely takes neither the write nor the MAX. I checked the second one specifically because a config/IO cost that turns out to sit in front of its own enabled check is the failure mode I just found on #1323; here the ordering is right and the comment at hook.py:1691 already says so.

The pooling concession is sound. exploration_events has no regime column and fire_idx is a single global schema_meta key with no scope dimension, so there is genuinely nothing to partition on. The direction argument is what carries it: the value is used only as a floor, so a pre-change row can only push the counter up. Worth keeping the "do not read this MAX as a measurement" sentence — that is the part a future reader would otherwise get wrong.

The healthy path is provably a no-op. The counter is bumped on every claim while record_exploration only writes on a turn that actually draws, so the ledger high-water mark is always ≤ the counter and max() never moves it. test_the_clamp_does_not_perturb_an_intact_counter pins that.

Acceptance, all four bullets: re-issue prevented (two arms), an arm that fails against current = 0 (verified by mutation, not by assertion), empty ledger still returns 1, docstring and code now agree.

On the _fire helper. Adding the DELETE ... WHERE fire_idx >= ? changes the fixture semantics of thirteen pre-existing call sites, so I traced the two that could have cared. _NOT_FIRING = _FIRING + 1 but a non-firing turn writes no ledger row, so the delete at _FIRING has nothing to remove there. test_a_firing_turn_writes_one_ledger_row... still sees exactly one row. And the determinism test's double _fire(_FIRING) is precisely the case the delete exists for — without it the second arm would have been clamped past the firing turn and first == second would have gone green against two non-firing turns. Correct change, and the docstring says why.

One note, no action: a hand-edited negative counter on an empty ledger still yields a non-positive index (-1 → 0, and should_explore(0) is True at any cadence). The never-decrease invariant survives it — the clamp makes each subsequent claim monotone from wherever it lands — so this is outside the issue's acceptance and not worth widening the PR for. Recording it only so it is not rediscovered as new.

What I changed

Rebase only — the branch was behind main after #1323 landed. No content changes; git range-diff is empty apart from the base. Both commits re-signed.

Verification

Full suite on the rebased tree: 6984 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. Discretion grep on added lines vs main: clean.

[github-actions]

merge-train: merged 050e4a6 → main via FF push.

[robotrocketscience]

[release:review:Setr:2026-08-04T05:01:47Z]