Repository navigation
recurate: a recuration of a span's middle leaves both sides, and the consumers run before a plan (#192) - #208
Merged
Merged
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideUpdates recuration tracking to preserve disjoint portions of queued and skipped spans, deduplicate claim operations, migrate legacy queue schemas, and run consumers before planning so pending recuration events cannot be sent twice. Sequence diagram for recuration planning after consumer drainsequenceDiagram
participant User
participant Recurate as curate::recurate
participant Worker as worker::run_once
participant Claims as Claims consumer
participant Planner as Recuration planner
User->>Recurate: recurate()
Recurate->>Worker: run_once()
Worker->>Claims: consume pending recuration
Claims->>Claims: subtract curated span from queued spans
Worker-->>Recurate: consumers drained
Recurate->>Planner: plan remaining spans
Planner-->>User: list or send recuration windows
Entity relationship diagram for recuration queue keyserDiagram
RECURATE {
string device
int from_seq
int to_seq
string op_device
int op_seq
}
CLAIM_OPERATION {
string op_device
int op_seq
}
CLAIM_OPERATION ||--o{ RECURATE : queues
Flow diagram for splitting queued and skipped spansflowchart LR
Original[Queued or skipped span 1-3] --> Recuration[Recuration of middle 2-2]
Recuration --> Before[Remaining part 1-1]
Recuration --> After[Remaining part 3-3]
Before --> Next[Plan only remaining spans]
After --> Next
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Up to standards ✅🟢 Issues
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00f6ec0502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ojungo69
added a commit
that referenced
this pull request
Sep 28, 2026
…r it, and held through the plan and its sending (review on #208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 017c94f561
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ojungo69
added a commit
that referenced
this pull request
Sep 28, 2026
… run once more whether it sent or not (review on #208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
|
You have reached your Codex usage limits for security reviews. Please try again later. |
ojungo69
added a commit
that referenced
this pull request
Sep 28, 2026
…#208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
This was referenced Sep 28, 2026
…des, and the consumers run before a plan (#192) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
…r it, and held through the plan and its sending (review on #208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
… run once more whether it sent or not (review on #208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
…#208) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
ojungo69
force-pushed
the
fix/recurate-parts
branch
from
September 28, 2026 09:46
74801de to
7751ed1
Compare
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
1 similar comment
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ojungo69
added a commit
that referenced
this pull request
Sep 28, 2026
The owner asked for this (2026-09-28): the open-code-review workflow's model effort goes from `max` to `high`. ## What `llm_reasoning_effort: max` becomes `high` in both review steps, the primary model and the fallback. `effort: high`, the action's own review depth, was already `high` and is unchanged. Nothing else changes. ## Note The review job failed on the recent PRs (#205, #206 and #208: "The primary review failed and no fallback review succeeded"). Whether `max` caused that has not been checked. The workflow runs from `main` (`pull_request_target`), so this PR's own run still uses `max`. The new value applies to runs after the merge. ## Review stop rule (declared before the first round) A finding is fixed here only if it shows the workflow would fail, or would run with weaker permissions or trust than today. Everything else goes to an issue. After two rounds in a row whose findings all go to issues, or a 👍, this PR is merged with no further commit once `check` and CodeQL are green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt ## Summary by Sourcery Set the open-code-review workflow to use high reasoning effort for both configured models. Enhancements: - Reduce the model reasoning effort used by both the primary and fallback open-code-review models from maximum to high while retaining the workflow’s high review depth. Documentation: - Update open-code-review documentation to reflect the high model reasoning effort and revised timeout rationale. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ojungo69
added a commit
that referenced
this pull request
Sep 28, 2026
…216) Closes #213. Two knowledge.db migrations in `claims::schema` checked the old schema and then changed it with no lock between the two. A worker and a command (`oboete recurate`, `oboete search`) opening the same older knowledge.db at once could both find the old schema; the second then failed on "duplicate column name: claim_at", or rebuilt the queue table a second time. ## What - `claims::migrate(k, needed, change)`: reads `needed` first, so a current knowledge.db takes no write lock. When a change is needed: - outside a transaction, it takes `BEGIN IMMEDIATE`, asks `needed` again, and changes only if it still holds; - inside one (a consumer's step or rewind, both immediate), the lock is already held, so it changes at once, as before. - The `claim_at` column (#210) and the queue's key rebuild (#208) both go through it. - `db::ensure_column`, which the issue named, takes `&mut Connection`; `schema` is called with the step's transaction (`&Connection`), so the same check-again shape is written here with `Transaction::new_unchecked`. ## Test `two_connections_migrate_one_older_knowledge_db_at_once`: for an older evidence table, then an older queue table, a first connection holds the write lock (as a step does) while a second runs `schema`; the first then migrates and commits. Both succeed, the column is there once, the queue has its three-column key, and the rows are kept. On `main`'s `claims.rs` it fails 3 runs in 3 with "duplicate column name: claim_at". ## Review stop rule (declared before the first round) A finding is fixed here only if a plain environment reaches it (a worker, a hook and the CLI commands on one home; Linux, macOS or Windows) and it loses a row, stops a worker run, or leaves the schema half changed. Anything else goes to an issue. After two rounds in a row whose findings all go to issues, this PR is merged with no further commit once `check` and CodeQL are green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt ## Summary by Sourcery Make knowledge.db schema migrations safe when workers and commands open an older database concurrently. Bug Fixes: - Prevent concurrent knowledge.db migrations from applying schema changes more than once or failing with duplicate-column errors. Enhancements: - Centralize migration checks so current databases avoid write locks while outdated databases recheck their schema under an immediate transaction. - Apply lock-safe migration handling to the claim_at column and recurate queue key migrations. Tests: - Add coverage for concurrent migrations of older evidence and queue schemas, verifying successful upgrades and preservation of rows. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #192. From the Codex review of #189 (rounds 7 and 8). In each case nothing was lost: a range already curated again was sent again, which spent a free cap, a subscription window or a paid entry's budget once more.
What
recuraterows are now keyed by(op_device, op_seq, from_seq), so one claim op's queued window can be split.coversfrom every queued span it touches. It keeps the parts before and after, and keeps whole a record it curated only in part, as before.queue()still queues an op once: an op already queued, whole or in the parts a recuration left, stays as it is. The old key did the same, and this way a trimmed row and a fresh whole row never overlap in a plan.claims::schema).skipped_spanssubtracts every later recuration withSpan::minus, which gives a part on each side, offsets and all.oboete recurateplans (item 3). A recuration appended before a crash or a failedrun_onceis read first, so its span is not listed or sent again. This isworker::run_once, the same pass--yesalready runs after sending.Tests
a_recuration_of_a_spans_middle_leaves_both_sides:1-3and skipped1-3, then a recuration of2-2: both give1-1and3-3;3-3leaves1-1;oboete recuratelists one span in one window.a_recuration_the_consumers_have_not_read_is_not_sent_again: a recuration appended with no consumer run afterwards.oboete recuratethen lists nothing.the_queue_of_an_older_knowledge_db_is_keyed_again: a table with the old key keeps its row and takes a second part of the same op.Review stop rule (declared before the first round)
A finding is fixed here only if a plain environment reaches it (the default or documented config, Linux, macOS or Windows) and it loses or duplicates a recuration. Everything else goes to an issue. After two rounds in a row whose findings all go to issues, this PR is merged with no further commit once
checkand CodeQL are green.🤖 Generated with Claude Code
https://claude.ai/code/session_012wbKzQZyG5Aq6poybwciHt
Summary by Sourcery
Preserve recuration span fragments and drain pending consumers before planning to prevent lost or duplicated recuration work.
New Features:
Bug Fixes:
Enhancements:
Tests: