Cherry-pick review testing - #450
Open
helq wants to merge 18 commits into
Open
Conversation
Add a submitted_at field to Event::RawMboxSubmitted, stamped with the server's current time when a raw mbox is submitted via POST /api/submit (Inject variant). This timestamp is propagated through metadata.received_date and used as the patchset date instead of the email's Date: header. This prevents stale mbox timestamps (which can be arbitrarily far in the past) from skewing queue ordering. The original email date is preserved in the messages table for display purposes. The Thread and Remote submission paths are unaffected as they already use server-side timestamps via create_fetching_patchset(). Signed-off-by: Elkin Cruz <elkin@google.com>
Define PriorityRule and CompiledPriorityRule structs for regex-based patchset priority classification. Add a custom serde deserializer (deserialize_indexed_vec) to handle both TOML array and env-var indexed-map representations. Add the priority_rules field to ReviewSettings with serde(default) so existing configs are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a priority INTEGER DEFAULT 500 column to the patchsets table and a composite index idx_patchsets_status_priority_date on (status, priority DESC, date ASC) for efficient priority-ordered queries. The column defaults to 500 so existing patchsets are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add migration to create the priority column and composite index. Introduce create_patchset_with_priority() which threads an explicit priority through all INSERT/UPDATE paths, with MIN(priority, ?) semantics to preserve manual deprioritization. Refactor create_patchset() to delegate with default None. Change get_pending_patchsets() ordering to priority DESC, date ASC. Add calculate_priority() for evaluating compiled regex rules against subjects (last match wins). Signed-off-by: Elkin Cruz <elkin@google.com>
Compile priority_rules from settings at startup and thread them through the DB worker into process_parsed_article(). Use calculate_priority() to compute priority from the patchset subject before calling create_patchset_with_priority(). API callers pass None for priority to create_fetching_patchset(). Signed-off-by: Elkin Cruz <elkin@google.com>
Expose the per-remote async mutex so that the fetcher module can acquire it before running git fetch, preventing concurrent fetches against the same remote (which causes duplicate multi-million-object downloads on large mirrors like the kernel repo). Signed-off-by: Elkin Cruz <elkin@google.com>
Introduce a fetch_queue table plus the DB operations that back the new durable fetch worker, modeled on the existing patchwork_outbox pattern. The table schema and FetchStatus/FetchQueueRow types are accompanied by the full set of queue operations: enqueue_fetch (idempotent), lock_pending_fetch (atomic claim under lease), mark_fetch_done, set_fetch_retry_at (backoff scheduling), mark_fetch_failed (terminal failure), sweep_ghost_fetches (reclaim expired leases), and get_stuck_fetch_placeholders (startup recovery for orphans). Also adds repo_url and priority columns to patchsets for crash recovery of in-flight fetches. This is the storage layer only; wiring lands in a follow-up. Signed-off-by: Elkin Cruz <elkin@google.com>
Add standalone utility functions that the upcoming FetchWorker will use, alongside the existing FetchAgent code: - PermanentFetchError: marker error for failures that should not be retried (e.g. missing commit with no remote to fetch from) - commit_hash_from_placeholder(): parse placeholder message ids (sha@sashiko.local or mr-N-base..head) into git commit/range - now_secs(): current unix timestamp helper - backoff_secs(): exponential backoff calculator (30s base, 10m cap) - looks_like_object_id(): detect full SHA vs ref name for efficient git fetch strategies Each function has unit tests. Signed-off-by: Elkin Cruz <elkin@google.com>
Rework git fetching around the fetch_queue table so requests survive restarts, cancellation, and worker crashes. FetchWorker polls the queue, claims due rows under a lease, ensures the commits are present (with an optimistic early-return when objects are already local, deliberately avoiding a full-remote fetch fallback that was pathologically slow on large mirrors), extracts patches, and emits PatchSubmitted events. Transient failures are retried with exponential backoff (30s doubling to a 10m cap); a fetch is only marked Failed after a continuous 24h retry window is exhausted. A periodic sweep reclaims ghost leases from dead workers. The per-remote lock (get_remote_lock) is acquired before every fetch to prevent concurrent fetches against the same remote, which previously caused duplicate multi-million-object downloads. The API submit paths now persist fetches via enqueue_fetch instead of pushing onto an mpsc channel, and the fetch_sender is removed from AppState/build_router/run_server. On startup, main backfills any patchset stuck in Fetching with no active queue row so in-flight work from before this change is recovered. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a nullable review_context column on patchsets that selects which review pipeline processes a patchset. NULL, the default, means the standard patch review; a JSON-encoded ReviewKind selects an alternate pipeline (currently cherry-pick review). The column is appended last so its NULL value is a free trailing field in SQLite, keeping patch-only deployments at zero extra storage. This commit defines only the storage shape and the enum; writing and reading the column arrive with their first users in later commits. Signed-off-by: Elkin Cruz <elkin@google.com>
Port the CherryPickContext type (renamed from the original hack's ConflictContext) and the finding filter into the new cherry_pick_review submodule. The filter drops low-severity and base-preexisting findings, drops original-patch-preexisting findings unless critical, and keeps resolution-introduced findings at medium or higher. The obsolete patch-body sentinel markers are dropped; the dispatch payload is persisted via the review_context column and ReviewKind instead. Includes filter and JSON round-trip unit tests. Signed-off-by: Elkin Cruz <elkin@google.com>
Add the cherry-pick-specific stage instructions, ported verbatim from the original hack so the pipeline stays behaviorally equivalent for benchmarking: - stage 1: semantic-intent analysis - stage 2: dropped-changes detection - stage 3: merge-correctness verification - origin classification (resolution_introduced, original_patch_preexisting, base_preexisting) The prompts are embedded via include_str so they ship with the binary regardless of the working directory. Shared stages 4-11 reuse the existing patch-review prompts and are not duplicated here. Signed-off-by: Elkin Cruz <elkin@google.com>
Express the merge-conflict resolution review (originally commit 66fb0a5) as a principled Pipeline over the shared stage machinery, instead of the inline hack in Worker::run. steps() runs cherry-specific analysis stages 1-3 (semantic intent, dropped changes, merge correctness) alongside the shared analysis stages 4-7, then the shared synthesis tail: dedup (8), concern resolution (9), verification (10), an origin-classification stage, a cherry-specific finding filter, and the final conflict report. The synthesis stages inject the accumulated concerns/findings into a per-stage prompt template. Those templates are ported verbatim from the original hack into synthesis.rs via the new StageSpec::with_template mechanism, so behaviour is preserved. The clean prompt equals the user prompt because it only affects logging, not the model call. build_context hydrates the three-commit context (subjects and the original patch diff) from git at review time, reusing the same static knowledge base and prefetched AST as the patch-review path. The database therefore only needs the minimal ReviewKind::CherryPick. Signed-off-by: Elkin Cruz <elkin@google.com>
Wire the alternate-pipeline selector end to end for the review binary. WorkerOptions gains a review_context field (Option<ReviewKind>), fed by a new --review-context JSON flag on the review CLI. When present in review_single_patch, the resolution commit is reviewed through the CherryPickReviewPipeline via execute_pipeline; when absent (the default), the existing Worker::run patch-review path is taken unchanged. The resolution SHA is the patch under review; the pipeline hydrates the rest of the three-commit context from git in build_context. This keeps the database footprint minimal: only the ReviewKind is threaded through. Signed-off-by: Elkin Cruz <elkin@google.com>
Add get_review_context / set_review_context to the Database, and have run_review_tool forward a patchset's stored review_context to the review subprocess via --review-context. Because run_review_tool already holds the database handle and patchset id, the selector is read there rather than threaded through every call site. Patchsets without a review_context (the default) are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a cherry-pick submit request that reviews an automated conflict resolution. It creates the usual fetching placeholder for the resolution commit, enqueues its fetch, and persists a minimal ReviewKind::CherryPick (original and optional base SHA) into review_context via set_review_context. The reviewer later forwards that selector to the pipeline, which hydrates the full three-commit context from git, so no bulky context is stored in the database. Signed-off-by: Elkin Cruz <elkin@google.com>
Introduce ReplayProvider test infrastructure for deterministic replay of model interactions and tool responses. Add integration equivalence tests in tests/cherry_pick_pipeline_equivalence.rs verifying six core execution paths through CherryPickReviewPipeline. Signed-off-by: Elkin Cruz <elkin@google.com>
Add edge case test suite for cherry-pick review pipeline covering validation retries, truncated responses, recitation handling, and error fallbacks. Introduce a data-driven replay test harness that discovers test cases from tests/fixtures/replays/*, executes real tool operations on ToolBox against synthetic or configured Git repositories, and asserts against golden output and history logs. Include the sample_cache synthetic fixture demonstrating a full end-to-end multi-turn review of a buffer cache refactor. Signed-off-by: Elkin Cruz <elkin@google.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
1 participant
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.
On top of #419
The top two commits include a series of tests for the cherry-pick review feature. The main consideration is that we can run sashiko and gather a trace, and then we can use this trace to prevent some regressions while in development. This strategy helped me develop the whole pipeline infrastructure by first experimenting with sashiko and changing everything on it.